fix: allow resolve_pull_request_review_thread on schedule/manual triggers when explicit thread_id is provided

[Copilot]

In default (legacy) mode, the handler always required triggeringPRNumber — causing it to reject calls from schedule/workflow_dispatch workflows even when an explicit thread_id had already been resolved to a specific PR via the GraphQL API.

Changes

• resolve_pr_review_thread.cjs: In the default/legacy else branch, replace the hard failure when triggeringPRNumber is absent with repo validation and an info log; skip the PR-number match check when there is no triggering PR. The check is preserved when a triggering PR is present. To maintain least-privilege, the legacy path now also validates the thread's repository against defaultTargetRepo/allowedRepos (via validateTargetRepo) and fails closed if the repo cannot be determined or is not allowed.

  // Before
  if (!triggeringPRNumber) {
    core.warning("Cannot resolve review thread: not running in a pull request context");
    return { success: false, error: "Cannot resolve review threads outside of a pull request context" };
  }
  
  // After (legacy mode now validates repo before proceeding)
  if (!threadRepo) {
    // fail closed — cannot allow resolving threads with unknown repo
  }
  const legacyRepoValidation = validateTargetRepo(threadRepo, defaultTargetRepo, allowedRepos);
  if (!legacyRepoValidation.valid) {
    // fail closed — thread repo not in allowed set
  }
  if (!triggeringPRNumber) {
    core.info(`No triggering PR context; resolving thread ${threadId} via explicit thread_id (PR #${threadPRNumber} in ${threadRepo})`);
  } else if (threadPRNumber !== triggeringPRNumber) {
    // existing mismatch guard, unchanged
  }

• resolve_pr_review_thread.test.cjs: Updated the existing "reject when not in a pull request context" test (now expects success with a valid thread_id in the default repo); added tests covering the schedule-trigger scenario with GraphQL call assertions, as well as failure cases for null thread repo and cross-repo threads in legacy mode.

[github-actions]

Hey @Copilot 👋 — great work on this fix! Allowing resolve_pull_request_review_thread to succeed on schedule/workflow_dispatch triggers when an explicit thread_id has already been resolved via the GraphQL API is a clean, targeted improvement to the agentic workflows infrastructure.

The PR is well-structured: the implementation change is minimal and surgical, the existing test is correctly updated to reflect the new expected behaviour, and a dedicated new test covers the schedule-trigger scenario with GraphQL call assertions. The PR description clearly explains the problem and the solution.

This looks ready for maintainer review! 🚀

---

This review was generated by the contribution-checker agent.

Generated by Contribution Check · ● 1.7M · ◷

[Copilot]

Pull request overview

This PR updates the resolve_pull_request_review_thread handler to allow resolving an explicitly provided thread_id even when the workflow was triggered without a pull request context (e.g., schedule / workflow_dispatch), and adjusts tests to cover the new behavior.

Changes:

• Relax legacy-mode behavior to no longer hard-fail when triggeringPRNumber is absent; instead logs and proceeds when thread_id is explicit.
• Update the “non-PR context” test to expect success with an explicit thread_id.
• Add a new test covering schedule-triggered behavior with GraphQL call assertions.

Show a summary per file

File	Description
actions/setup/js/resolve_pr_review_thread.cjs	Adjusts legacy-mode scoping logic to permit thread resolution without a triggering PR number when thread_id is provided.
actions/setup/js/resolve_pr_review_thread.test.cjs	Updates/extends tests to validate success paths for non-PR contexts (including schedule triggers).

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 2/2 changed files
• Comments generated: 1

[Copilot]

In legacy mode (no explicit target-repo/allowed_repos), this new branch allows resolving a thread when triggeringPRNumber is missing without any repository scoping. If a per-handler github-token is configured (PAT/App) with access to multiple repos, a schedule/workflow_dispatch run could now resolve review threads in other repositories, bypassing the repo allowlist logic that only runs when hasExplicitTargetConfig is true.

To keep the behavior least-privilege while still supporting schedule/manual triggers, add a repo validation in the legacy path (e.g., require threadRepo and ensure it matches defaultTargetRepo, or call validateTargetRepo(threadRepo, defaultTargetRepo, allowedRepos) even when hasExplicitTargetConfig is false) and fail closed on mismatch/unknown repo.

See below for a potential fix:

        // Default (legacy) mode: keep least-privilege behavior by requiring the resolved thread
        // to belong to the default/allowed repository set even when there is no triggering PR.
        if (!threadRepo) {
          core.warning(`Unable to determine repository for review thread ${threadId}; refusing to resolve in legacy mode`);
          return {
            success: false,
            error: `Unable to determine repository for review thread ${threadId}`,
          };
        }

        const legacyRepoValidation = validateTargetRepo(threadRepo, defaultTargetRepo, allowedRepos);
        if (!legacyRepoValidation.valid) {
          core.warning(`Thread ${threadId} repository ${threadRepo} is not allowed in legacy mode`);
          return {
            success: false,
            error: legacyRepoValidation.error || `Repository ${threadRepo} is not allowed for this handler`,
          };
        }

        // Scope to triggering PR only when a triggering PR exists
        if (!triggeringPRNumber) {
          // No triggering PR (e.g. schedule/workflow_dispatch trigger), but the thread has been
          // resolved to a specific allowed repository via the API — allow the resolution to proceed
          core.info(`No triggering PR context; resolving thread ${threadId} via explicit thread_id (PR #${threadPRNumber} in ${threadRepo})`);

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 80/100

✅ Excellent

Metric	Value
New/modified tests analyzed	2
✅ Design tests (behavioral contracts)	2 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	2 (100%)
Duplicate test clusters	0
Test inflation detected	⚠️ Yes (29 test additions / 5 production additions = 5.8:1)
🚨 Coding-guideline violations	None

---

Test Classification Details

📋 All 2 modified/new tests

Test	File	Classification	Issues Detected
should succeed when not in a pull request context but explicit thread_id is provided	resolve_pr_review_thread.test.cjs	✅ Design	None — verifies observable outputs (success, thread_id, is_resolved)
should succeed when triggered by a schedule event (no PR context) with explicit thread_id	resolve_pr_review_thread.test.cjs	✅ Design	Minor: toHaveBeenCalledTimes(2) checks internal API call count alongside behavioral assertions

---

Test Analysis

Test 1 (modified): should succeed when not in a pull request context but explicit thread_id is provided

This test was renamed and updated from the previous should reject when not in a pull request context — correctly reflecting the new behavioral contract. It verifies that when an explicit thread_id is provided and the workflow runs outside a PR context (e.g., a workflow_dispatch or push event), the resolution succeeds. Assertions are purely behavioral: result.success, result.thread_id, result.is_resolved. The test itself exercises an edge case (non-PR event context), which is exactly the scenario the fix addresses.

Test 2 (new): should succeed when triggered by a schedule event (no PR context) with explicit thread_id

Adds specific coverage for schedule-triggered workflows (empty context.payload). Behavioral assertions are solid. The additional expect(mockGraphql).toHaveBeenCalledTimes(2) and toHaveBeenCalledWith(...) assertions verify that both a thread-lookup query and a resolve mutation were issued — this is acceptable because it confirms the resolution actually reached the external API rather than short-circuiting. The mock target (global.github.graphql) is an external I/O boundary, not an internal business-logic function.

---

Test Inflation Note

The raw addition ratio (29 test lines / 5 production lines = 5.8:1) triggers the inflation flag, but the context is benign: the production change was primarily a simplification (9 lines deleted, 5 added), while the tests expand coverage for two new valid trigger scenarios. The added test lines are not padding — they are meaningful assertions directly tied to the behavioral change.

---

Language Support

Tests analyzed:

• 🟨 JavaScript (*.test.cjs): 2 tests (vitest)

No Go test files were changed in this PR.

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). Both modified/new tests verify observable behavioral contracts for the edge case this PR fixes. The test inflation flag is a measurement artifact of a primarily deletion-based production change; the new test lines add genuine coverage.

---

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 397.7K · ◷

[github-actions]

✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Both changed tests verify behavioral contracts for the schedule/non-PR trigger edge case this PR fixes.

[pelikhan]

@copilot review comments

[Copilot]

@copilot review comments

Addressed in 68d2015. The legacy mode path now validates the thread's repository against defaultTargetRepo/allowedRepos (via validateTargetRepo) before allowing schedule-triggered resolution — fails closed if the repo cannot be determined or is not in the allowed set. Added two new tests covering both failure scenarios.

[Ejezzy9]

good work keep up