fix(ship): address coderabbit critical findings on pr #75

- resolve-review-thread mcp: do a read-only preflight query to verify
  the thread belongs to the bound pr before issuing the resolve mutation.
  the previous flow ran the mutation first and only rejected
  cross-pr threads after the side effect had already happened.

- eligibility: fr-015/fr-028 require the gate to evaluate the
  triggering principal only. drop the ownerOk fallback, which let any
  commenter trigger the bot whenever the repo owner happened to be in
  ALLOWED_OWNERS.

- tickle-scheduler: requeue the intent on dispatch failure. ZREM ran
  before onDue, so a thrown onDue stranded the session until startup
  reconciliation re-ran. requeue with a one-interval delay keeps the
  session live without tight-looping a known-bad dispatch.

- test-isolated.sh: fail fast when the test glob matches nothing.
  nullglob silently turned an empty match into a successful no-op,
  hiding a deleted test directory or broken glob behind a green ci.

Author: chrisleekr

scripts/test-isolated.sh

@@ -9,7 +9,13 @@ passed=0
 failed=0
 failures=()
 
-for f in test/**/*.test.ts; do
+tests=(test/**/*.test.ts)
+if (( ${#tests[@]} == 0 )); then
+  echo "No test files matched test/**/*.test.ts" >&2
+  exit 1
+fi
+
+for f in "${tests[@]}"; do
   output=$(bun test "$f" 2>&1)
   has_zero_fail=false
   has_skip=false

src/mcp/servers/resolve-review-thread.ts

@@ -51,6 +51,19 @@ const server = new McpServer({
   version: "1.0.0",
 });
 
+const GET_THREAD_QUERY = `
+  query GetReviewThread($threadId: ID!) {
+    node(id: $threadId) {
+      ... on PullRequestReviewThread {
+        id
+        pullRequest {
+          number
+        }
+      }
+    }
+  }
+`;
+
 const RESOLVE_MUTATION = `
   mutation ResolveReviewThread($threadId: ID!) {
     resolveReviewThread(input: { threadId: $threadId }) {
@@ -65,6 +78,13 @@ const RESOLVE_MUTATION = `
   }
 `;
 
+interface PreflightResponse {
+  node: {
+    id: string;
+    pullRequest: { number: number };
+  } | null;
+}
+
 interface ResolveResponse {
   resolveReviewThread: {
     thread: {
@@ -117,19 +137,21 @@ server.tool(
   },
   async ({ thread_id }) => {
     try {
-      const result = await octokit.graphql<ResolveResponse>(RESOLVE_MUTATION, {
+      const preflight = await octokit.graphql<PreflightResponse>(GET_THREAD_QUERY, {
         threadId: thread_id,
       });
-      const thread = result.resolveReviewThread.thread;
-
-      if (thread.pullRequest.number !== BOUND_PR_NUMBER) {
+      const preflightPr = preflight.node?.pullRequest.number;
+      if (preflightPr !== BOUND_PR_NUMBER) {
         return {
           content: [
             {
               type: "text" as const,
               text: JSON.stringify({
                 code: "graphql_error",
-                message: `thread belongs to PR #${String(thread.pullRequest.number)} but this server is bound to PR #${String(BOUND_PR_NUMBER)}`,
+                message:
+                  preflightPr === undefined
+                    ? `thread ${thread_id} not found or not a PullRequestReviewThread`
+                    : `thread belongs to PR #${String(preflightPr)} but this server is bound to PR #${String(BOUND_PR_NUMBER)}`,
                 thread_id,
               }),
             },
@@ -138,6 +160,11 @@ server.tool(
         };
       }
 
+      const result = await octokit.graphql<ResolveResponse>(RESOLVE_MUTATION, {
+        threadId: thread_id,
+      });
+      const thread = result.resolveReviewThread.thread;
+
       return {
         content: [
           {

src/workflows/ship/eligibility.ts

@@ -69,8 +69,7 @@ export async function checkEligibility(input: EligibilityInput): Promise<Eligibi
     const triggerOk = allowedOwners.some(
       (o) => o.toLowerCase() === input.triggeringUserLogin.toLowerCase(),
     );
-    const ownerOk = allowedOwners.some((o) => o.toLowerCase() === input.owner.toLowerCase());
-    if (!triggerOk && !ownerOk) {
+    if (!triggerOk) {
       return {
         eligible: false,
         reason: "unauthorized",

src/workflows/ship/tickle-scheduler.ts

@@ -75,9 +75,12 @@ export function createTickleScheduler(deps: TickleSchedulerDeps): TickleSchedule
       try {
         await deps.onDue(intent_id);
       } catch (err) {
+        // Re-arm so a transient dispatch failure doesn't strand the
+        // session until startup reconciliation runs again.
+        await deps.valkey.send("ZADD", [TICKLE_KEY, String(Date.now() + intervalMs), intent_id]);
         logger.error(
           { event: "ship.tickle.dispatch_failed", intent_id, err: String(err) },
-          "ship tickle dispatch failed",
+          "ship tickle dispatch failed; requeued",
         );
       }
     }