fix: rivet binary path resolution + drop fragile .issues guard#29
Merged
fix: rivet binary path resolution + drop fragile .issues guard#29
Conversation
## 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two production bugs surfaced by today's live test
Bug 1 — rivet binary looked up in the wrong cwd
Logs at 20:14:31 UTC: `err: 'spawn data/rivet/rivet ENOENT'`. `runRivetOracle` calls `execFile(binary, args, { cwd: repoPath })` where `repoPath` is the freshly-extracted PR tarball. A relative `binary_path` is resolved against THAT cwd, not the temper deploy root. Node looks for `/data/rivet/rivet` instead of `/opt/temper/data/rivet/rivet`.
Fix: resolve a relative path against `__dirname/..` before passing to the oracle.
Bug 3 — `/review-pr` silently swallowed
The `issue_comment.created` handler had:
```js
if (!comment?.body || !context.octokit?.issues) return;
```
Same Probot v14 issue as PR #22: `context.octokit.issues` is sometimes `undefined` for this event type. The guard then silently kills every ChatOps command. Today's `/review-pr` on rivet#213 and temper#28 hit this — webhook arrived, handler bailed in 1 ms, no log.
Fix: drop the `.issues` portion of the guard. Downstream calls that work today keep working; one that fails throws into `onError` instead of vanishing.
Test plan
Risk & rollout
🤖 Generated with Claude Code