From 88cf21c8869ef34461d53c6bce024a5772ae26ea Mon Sep 17 00:00:00 2001 From: aaight Date: Thu, 6 Aug 2026 14:03:29 +0200 Subject: [PATCH] feat(triggers): extend authorMode to respond-to-ci and resolve-conflicts + fork write-access skip (#1535) Co-authored-by: Cascade Bot --- CHANGELOG.md | 2 + docs/architecture/03-trigger-system.md | 14 +- docs/getting-started.md | 11 ++ src/agents/definitions/resolve-conflicts.yaml | 7 + src/agents/definitions/respond-to-ci.yaml | 7 + src/github/client.ts | 28 +++ src/triggers/github/check-suite-decision.ts | 67 +++---- src/triggers/github/check-suite-failure.ts | 53 +++--- src/triggers/github/pr-conflict-detected.ts | 45 +++-- src/triggers/github/respond-to-ci-dispatch.ts | 19 +- src/triggers/github/utils.ts | 60 +----- src/triggers/shared/author-mode.ts | 79 ++++++++ src/triggers/shared/gates.ts | 73 ++++++++ tests/unit/github/client.test.ts | 76 ++++++++ .../unit/triggers/check-suite-failure.test.ts | 171 +++++++++++++++++- .../github/check-suite-decision.test.ts | 78 ++++++++ .../triggers/pr-conflict-detected.test.ts | 114 +++++++++++- .../unit/triggers/shared/author-mode.test.ts | 61 +++++++ tests/unit/triggers/shared/gates.test.ts | 96 ++++++++++ 19 files changed, 915 insertions(+), 146 deletions(-) create mode 100644 src/triggers/shared/author-mode.ts create mode 100644 tests/unit/triggers/shared/author-mode.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b126d92..42861cc9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ All notable user-visible changes to CASCADE are documented here. The format is l ### Added +- **`authorMode` (own/external/all) extended to the CI-failure and conflict triggers, plus a fork write-access skip** ([MNG-1774](https://linear.app/issue/MNG-1774)). The `authorMode` trigger parameter — previously only on `review` (`scm:check-suite-success` / `scm:pr-opened`) — now also applies to `respond-to-ci` (`scm:check-suite-failure`) and `resolve-conflicts` (`scm:pr-conflict-detected`), surfaced in the metadata-driven trigger UI via the two agent YAMLs. It defaults to `own`, so existing projects are unchanged: `own` restricts dispatch to CASCADE-authored PRs, while `external`/`all` extend it to human-authored **same-repo** PRs. Because both agents *push commits* and CASCADE has no write access to a contributor's fork, a new shared `gateForkWriteAccess` gate turns a fork PR (under `external`/`all`) into a clean, self-explanatory skip (`PR #N head branch lives on fork — CASCADE has no write access to push fixes`) instead of firing an agent that fails mid-run at `git push`. Fork status comes from a small `getPR`/`PRDetails` extension (`isFork` + `headRepoFullName`). The author-mode logic — previously three near-duplicate copies — is consolidated into a single shared module at `src/triggers/shared/author-mode.ts` that every hard-gated caller (review, respond-to-ci, resolve-conflicts) delegates to. Set it with `cascade projects trigger-set --agent respond-to-ci --event scm:check-suite-failure --enable --params '{"authorMode":"all"}'` (or `--agent resolve-conflicts --event scm:pr-conflict-detected`). **Caution:** `all`/`external` authorize CASCADE to write to human-authored same-repo branches — a conscious opt-in. Closes [MNG-1774](https://linear.app/mongrel/issue/MNG-1774). + - **Dashboard: build a project's worker image from a Dockerfile, with a source selector, live build status, and Rebuild** ([MNG-1725](https://linear.app/issue/MNG-1725), spec 023 plan 5 of 5 — final). The **Worker Image** card in **Project → Settings → General** now lets a **superadmin** choose the image **source** — **Global default**, **Referenced image** (the spec-022 control, unchanged), or **Dockerfile**. For the Dockerfile source, paste **only the extra layers** (RUN / COPY / ENV …) into a textarea and **Set** (calls `projects.update({workerDockerfile})`) — CASCADE supplies the pinned `FROM cascade-worker` base and builds the image router-side. The two override sources are **mutually exclusive**: selecting one hides the other's control, matching the backend invariant. The status display separates the **active image** (`workerImageStatus`: pending / building / verified / failed) from the **most recent build attempt** (`workerImageBuildStatus`: building / failed), so a project running its last-good image while a rebuild fails reads **"Verified … · last rebuild failed: ``"** rather than a misleading "Failed"; a **Building…** spinner shows for a first build and the card **polls** (`WORKER_IMAGE_POLL_MS`) while `workerImageStatus === 'building' || workerImageBuildStatus === 'building'`. A **Rebuild** button (Dockerfile source only) calls `projects.rebuildWorkerImage` to re-run the build against a refreshed base without editing the content. The whole card stays hidden for non-superadmins. Operator docs (`README.md`, `docs/getting-started.md`) walk through writing extra layers → save → watch build/verify (or read a failure) → rebuild, and call out the mutual exclusivity and the **single-daemon constraint** (a Dockerfile-built image is local to the router that built it). Completes the worker-Dockerfile feature end-to-end across schema, spawn resolution, build engine, set surfaces, and dashboard. Closes [MNG-1725](https://linear.app/mongrel/issue/MNG-1725). - **Dashboard: set/clear a project's worker image with live verified/pending/failed status** ([MNG-1699](https://linear.app/issue/MNG-1699), spec 022 plan 4 of 4 — final). A **superadmin** can now manage a project's per-project worker image from the dashboard: a new **Worker Image** card in **Project → Settings → General** shows the global default as the input placeholder, accepts a reference (**Set**), reverts to the global default (**Clear**), and reflects the router-side validation lifecycle inline — a **Verifying…** spinner that polls while `pending` (same approach as the run-status pages), a **Verified — pinned to `@sha256:…`** badge once the digest is resolved, or a **Validation failed: ``** badge naming the missing requirement. The control is wired to the existing `projects.update` mutation (set sends `workerImage`, clear sends `null`) and is hidden entirely for non-superadmins, mirroring the backend gate. Completes the feature end-to-end across CLI, API, and dashboard; the operator walkthrough in `docs/getting-started.md` covers deriving a custom image `FROM` the Cascade worker base, making it available in both the registry-backed and self-hosted/local topologies, setting it from the dashboard, and confirming `verified`. Closes [MNG-1699](https://linear.app/mongrel/issue/MNG-1699). diff --git a/docs/architecture/03-trigger-system.md b/docs/architecture/03-trigger-system.md index f5d6900f..2b11ab15 100644 --- a/docs/architecture/03-trigger-system.md +++ b/docs/architecture/03-trigger-system.md @@ -115,14 +115,24 @@ function registerBuiltInTriggers(registry: TriggerRegistry): void { | Handler | Event | Agent | |---------|-------|-------| | `CheckSuiteSuccessTrigger` | CI passed | `review` (with `authorMode` param) | -| `CheckSuiteFailureTrigger` | CI failed | `respond-to-ci` | +| `CheckSuiteFailureTrigger` | CI failed | `respond-to-ci` (with `authorMode` param) | | `PrReviewSubmittedTrigger` | Review with changes_requested | `respond-to-review` | | `ReviewRequestedTrigger` | Bot requested as reviewer | `review` | | `PrOpenedTrigger` | PR opened | `review` | | `PrCommentMentionTrigger` | Bot @mentioned in PR comment | `respond-to-pr-comment` | | `PrMergedTrigger` | PR merged | PM status update (no agent) | | `PrReadyToMergeTrigger` | PR approved + checks pass | PM status update (no agent) | -| `PrConflictDetectedTrigger` | Merge conflict on PR | `resolve-conflicts` | +| `PrConflictDetectedTrigger` | Merge conflict on PR | `resolve-conflicts` (with `authorMode` param) | + +The `authorMode` parameter (`own` / `external` / `all`, default `own`) filters PRs +by author type for `CheckSuiteSuccessTrigger`, `CheckSuiteFailureTrigger`, and +`PrConflictDetectedTrigger`. `own` restricts dispatch to CASCADE-authored PRs +(the historical behavior); `external`/`all` extend it to human-authored PRs. The +shared evaluator lives in `src/triggers/shared/author-mode.ts`. Because +`respond-to-ci` and `resolve-conflicts` *push commits* and CASCADE has no write +access to a contributor's fork, both apply a **fork write-access skip** +(`gateForkWriteAccess`) — a fork PR under `external`/`all` produces a clean, +self-explanatory skip instead of failing mid-run at `git push`. ### Linear triggers (`src/triggers/linear/`) diff --git a/docs/getting-started.md b/docs/getting-started.md index 84b301a7..f002f420 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -380,6 +380,17 @@ node bin/cascade.js projects trigger-set my-project \ node bin/cascade.js projects trigger-set my-project \ --agent respond-to-ci --event scm:check-suite-failure --enable +# respond-to-ci and resolve-conflicts also accept the `authorMode` param +# (own/external/all, default own). `all`/`external` authorize CASCADE to WRITE +# to human-authored (same-repo) branches — a conscious opt-in. Fork PRs are +# always skipped (CASCADE cannot push to a contributor's fork). +node bin/cascade.js projects trigger-set my-project \ + --agent respond-to-ci --event scm:check-suite-failure --enable \ + --params '{"authorMode":"all"}' +node bin/cascade.js projects trigger-set my-project \ + --agent resolve-conflicts --event scm:pr-conflict-detected --enable \ + --params '{"authorMode":"all"}' + # Enable respond-to-review when the reviewer requests changes node bin/cascade.js projects trigger-set my-project \ --agent respond-to-review --event scm:pr-review-submitted --enable diff --git a/src/agents/definitions/resolve-conflicts.yaml b/src/agents/definitions/resolve-conflicts.yaml index 6753d975..2c724d95 100644 --- a/src/agents/definitions/resolve-conflicts.yaml +++ b/src/agents/definitions/resolve-conflicts.yaml @@ -31,6 +31,13 @@ triggers: description: Trigger when a PR has merge conflicts with the base branch defaultEnabled: false providers: [github] + parameters: + - name: authorMode + type: select + label: Author Filter + description: Filter PRs by author type + options: [own, external, all] + defaultValue: own contextPipeline: [prContext, directoryListing, contextFiles, workItem] strategies: {} diff --git a/src/agents/definitions/respond-to-ci.yaml b/src/agents/definitions/respond-to-ci.yaml index d67138f2..5f80f2bc 100644 --- a/src/agents/definitions/respond-to-ci.yaml +++ b/src/agents/definitions/respond-to-ci.yaml @@ -32,6 +32,13 @@ triggers: description: Trigger when CI checks fail defaultEnabled: false providers: [github] + parameters: + - name: authorMode + type: select + label: Author Filter + description: Filter PRs by author type + options: [own, external, all] + defaultValue: own contextPipeline: [prContext, directoryListing, contextFiles, workItem] strategies: {} diff --git a/src/github/client.ts b/src/github/client.ts index b397ffa9..66b44dbd 100644 --- a/src/github/client.ts +++ b/src/github/client.ts @@ -31,6 +31,19 @@ export interface PRDetails { merged: boolean; mergeable: boolean | null; user: { login: string }; + /** + * Full name (`owner/repo`) of the PR's head repository, or `null` when the + * head repo is unavailable (deleted fork). Optional so existing `getPR` + * mocks keep type-checking and default to "not a fork". + */ + headRepoFullName?: string | null; + /** + * True when the PR's head branch lives on a different repo than the base + * (a fork). CASCADE cannot push commits to a contributor's fork, so + * write-mode agents (respond-to-ci, resolve-conflicts) skip fork PRs + * cleanly instead of failing at push. Optional and defaults to non-fork. + */ + isFork?: boolean; } export interface PRReviewComment { @@ -162,6 +175,19 @@ export const githubClient = { repo, pull_number: prNumber, }); + // Fork detection. The `null` (deleted fork) vs `undefined` (test mock) + // split below is DELIBERATE — do not collapse them: + // - head.repo === null → the fork was deleted; it is unpushable, + // so treat as a fork (skip write agents). + // - head.repo present → fork iff its full_name differs from base. + // - head.repo === undefined → test-mock payloads without a head repo; + // treat as non-fork so the many existing + // getPR mocks default to "not a fork". + const baseRepoFullName = data.base.repo?.full_name ?? `${owner}/${repo}`; + const headRepo = data.head.repo; + const headRepoFullName = headRepo?.full_name ?? null; + const isFork = + headRepo === null ? true : headRepo ? headRepo.full_name !== baseRepoFullName : false; return { number: data.number, title: data.title, @@ -174,6 +200,8 @@ export const githubClient = { merged: data.merged ?? false, mergeable: data.mergeable ?? null, user: { login: data.user?.login || 'unknown' }, + headRepoFullName, + isFork, }; }, diff --git a/src/triggers/github/check-suite-decision.ts b/src/triggers/github/check-suite-decision.ts index a20939ab..9b6c9823 100644 --- a/src/triggers/github/check-suite-decision.ts +++ b/src/triggers/github/check-suite-decision.ts @@ -1,6 +1,7 @@ import type { CheckSuiteStatus } from '../../github/client.js'; import { isCascadeBot, type PersonaIdentities } from '../../github/personas.js'; import type { ProjectConfig } from '../../types/index.js'; +import { evaluateAuthorMode } from '../shared/author-mode.js'; export type CheckSuiteDecision = | { action: 'defer'; incompleteChecks: string[]; message: string } @@ -10,7 +11,7 @@ export type CheckSuiteDecision = export type CheckSuiteDecisionMode = | { kind: 'review'; parameters: Record } - | { kind: 'respond-to-ci' }; + | { kind: 'respond-to-ci'; parameters: Record }; export interface DecideCheckSuiteOutcomeOptions { prNumber: number; @@ -27,68 +28,48 @@ export interface DecideCheckSuiteAggregateOptions extends DecideCheckSuiteOutcom } const FAILURE_CONCLUSIONS = new Set(['failure', 'timed_out', 'action_required']); -const VALID_AUTHOR_MODES = new Set(['own', 'external', 'all']); - -function resolveAuthorMode(parameters: Record): string { - const rawMode = parameters.authorMode; - return typeof rawMode === 'string' && VALID_AUTHOR_MODES.has(rawMode) ? rawMode : 'own'; -} +/** + * Thin wrapper over the shared `evaluateAuthorMode`, adapting its result into a + * check-suite `skip` decision. Preserves the established skip-message text + * (including the `isCascadePR=` suffix) so webhook decision reasons are stable. + */ function authorModeDecision( prAuthorLogin: string, personaIdentities: PersonaIdentities | undefined, parameters: Record, prNumber: number, + handlerName: string, ): Extract | null { - if (!personaIdentities) { + const result = evaluateAuthorMode(prAuthorLogin, personaIdentities, parameters, handlerName); + if (!result) { return { action: 'skip', message: 'Cascade persona identities could not be resolved (token / GitHub API issue)', }; } - - const authorMode = resolveAuthorMode(parameters); - const isCascadePR = isCascadeBot(prAuthorLogin, personaIdentities); - const shouldTrigger = - authorMode === 'all' || - (authorMode === 'own' && isCascadePR) || - (authorMode === 'external' && !isCascadePR); - - if (shouldTrigger) return null; - + if (result.shouldTrigger) return null; return { action: 'skip', - message: `PR #${prNumber} author ${prAuthorLogin} does not match configured authorMode '${authorMode}' (isCascadePR=${isCascadePR})`, - }; -} - -function cascadePersonaDecision( - prAuthorLogin: string, - personaIdentities: PersonaIdentities | undefined, - prNumber: number, -): Extract | null { - if (!personaIdentities) { - return { - action: 'skip', - message: 'Cascade persona identities could not be resolved (token / GitHub API issue)', - }; - } - if (isCascadeBot(prAuthorLogin, personaIdentities)) return null; - return { - action: 'skip', - message: `PR #${prNumber} not authored by a cascade persona (author: ${prAuthorLogin})`, + message: `PR #${prNumber} author ${prAuthorLogin} does not match configured authorMode '${result.authorMode}' (isCascadePR=${result.isCascadePR})`, }; } export function decideCheckSuiteGates( options: DecideCheckSuiteOutcomeOptions, ): Extract | null { - const { prNumber, prAuthorLogin, prBaseRef, project, personaIdentities, mode } = options; - - const authorSkip = - mode.kind === 'review' - ? authorModeDecision(prAuthorLogin, personaIdentities, mode.parameters, prNumber) - : cascadePersonaDecision(prAuthorLogin, personaIdentities, prNumber); + const { prNumber, prAuthorLogin, prBaseRef, project, personaIdentities, handlerName, mode } = + options; + + // Both `review` and `respond-to-ci` modes now carry authorMode parameters + // and route through the shared author-mode evaluator (MNG-1774). + const authorSkip = authorModeDecision( + prAuthorLogin, + personaIdentities, + mode.parameters, + prNumber, + handlerName, + ); if (authorSkip) return authorSkip; // Bug 2 (2026-05-11 prod incident on ucho PR #393, MNG-691): diff --git a/src/triggers/github/check-suite-failure.ts b/src/triggers/github/check-suite-failure.ts index f1f0b9c2..b0cdad1f 100644 --- a/src/triggers/github/check-suite-failure.ts +++ b/src/triggers/github/check-suite-failure.ts @@ -2,11 +2,10 @@ import { githubClient } from '../../github/client.js'; import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; import { parseRepoFullName } from '../../utils/repo.js'; -import { gateCascadePersona, requirePersonaIdentities } from '../shared/gates.js'; import { buildDeferredRecheckResult } from '../shared/result-builders.js'; import { skip } from '../shared/skip.js'; -import { checkTriggerEnabled } from '../shared/trigger-check.js'; -import { decideCheckSuiteOutcome } from './check-suite-decision.js'; +import { checkTriggerEnabledWithParams } from '../shared/trigger-check.js'; +import { decideCheckSuiteGates, decideCheckSuiteOutcome } from './check-suite-decision.js'; import { resolveCheckSuitePRNumber } from './pr-resolution.js'; import { dispatchRespondToCi, resetFixAttempts } from './respond-to-ci-dispatch.js'; import { type GitHubCheckSuitePayload, isGitHubCheckSuitePayload } from './types.js'; @@ -40,14 +39,14 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { // Disabled-at-config returns null so the registry's first-match loop // continues to the next matcher — see `src/triggers/shared/trigger-check.ts` // for the disabled-shadowing contract. - if ( - !(await checkTriggerEnabled( - ctx.project.id, - 'respond-to-ci', - 'scm:check-suite-failure', - this.name, - )) - ) { + // Check trigger config + get parameters (authorMode) in a single DB call. + const triggerConfig = await checkTriggerEnabledWithParams( + ctx.project.id, + 'respond-to-ci', + 'scm:check-suite-failure', + this.name, + ); + if (!triggerConfig.enabled) { return null; } @@ -72,20 +71,24 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { // Fetch PR details const prDetails = await githubClient.getPR(owner, repo, prNumber); - const personasResult = requirePersonaIdentities(ctx.personaIdentities, prNumber, this.name); - if (!personasResult.ok) return personasResult.skip; - - // Cascade-authored PRs bypass the base-branch gate — a cascade PR - // targeting a non-base branch is a stacked PR, not a drive-by. - // Non-cascade authors are filtered here and never reach the gate. - // Mirrors the authorIsCascade bypass in decideCheckSuiteGates (lines 101-109). - const cascadePersonaSkip = gateCascadePersona( - prDetails.user.login, + // Author-mode + base-branch gate BEFORE the checks API call (preserves + // the pre-API skip; mirrors check-suite-success). Handles the missing- + // personaIdentities case internally, so no separate requirePersonaIdentities + // call is needed here. `own` (default) filters to cascade-authored PRs; + // `external`/`all` now dispatch respond-to-ci on human same-repo PRs. + const mode = { kind: 'respond-to-ci', parameters: triggerConfig.parameters } as const; + const gateSkip = decideCheckSuiteGates({ prNumber, - personasResult.value, - this.name, - ); - if (cascadePersonaSkip) return cascadePersonaSkip; + prAuthorLogin: prDetails.user.login, + prBaseRef: prDetails.baseRef, + project: ctx.project, + personaIdentities: ctx.personaIdentities, + handlerName: this.name, + mode, + }); + if (gateSkip) { + return skip(this.name, gateSkip.message); + } // Resolve work item from DB const workItemId = await resolveWorkItemId(ctx.project.id, prNumber); @@ -100,7 +103,7 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { project: ctx.project, personaIdentities: ctx.personaIdentities, handlerName: this.name, - mode: { kind: 'respond-to-ci' }, + mode, }); if (decision.action === 'defer') { diff --git a/src/triggers/github/pr-conflict-detected.ts b/src/triggers/github/pr-conflict-detected.ts index ef803501..d6568487 100644 --- a/src/triggers/github/pr-conflict-detected.ts +++ b/src/triggers/github/pr-conflict-detected.ts @@ -4,13 +4,14 @@ import { logger } from '../../utils/logging.js'; import { parseRepoFullName } from '../../utils/repo.js'; import { gateAttemptLimit, + gateAuthorMode, gateBaseBranch, - gateCascadePersona, + gateForkWriteAccess, requirePersonaIdentities, } from '../shared/gates.js'; import { buildDeferredRecheckResult } from '../shared/result-builders.js'; import { skip } from '../shared/skip.js'; -import { checkTriggerEnabled } from '../shared/trigger-check.js'; +import { checkTriggerEnabledWithParams } from '../shared/trigger-check.js'; import { buildResolveConflictsResult } from './result-builders.js'; import { type GitHubPullRequestPayload, isGitHubPullRequestPayload } from './types.js'; import { resolveWorkItemId } from './utils.js'; @@ -67,14 +68,14 @@ export class PRConflictDetectedTrigger implements TriggerHandler { // Disabled-at-config returns null so the registry's first-match loop // continues to the next matcher — see `src/triggers/shared/trigger-check.ts` // for the disabled-shadowing contract. - if ( - !(await checkTriggerEnabled( - ctx.project.id, - 'resolve-conflicts', - 'scm:pr-conflict-detected', - this.name, - )) - ) { + // Check trigger config + get parameters (authorMode) in a single DB call. + const triggerConfig = await checkTriggerEnabledWithParams( + ctx.project.id, + 'resolve-conflicts', + 'scm:pr-conflict-detected', + this.name, + ); + if (!triggerConfig.enabled) { return null; } @@ -83,17 +84,21 @@ export class PRConflictDetectedTrigger implements TriggerHandler { const repoFullName = payload.repository.full_name; const { owner, repo } = parseRepoFullName(repoFullName); - // Sync gate chain — author must be a cascade persona AND the PR must - // target the project's base branch. Loop-prevention: only auto-resolve - // conflicts on PRs authored by bot personas; human PRs are owned by - // the human. + // Sync gate chain — author must match the configured authorMode AND the + // PR must target the project's base branch. `own` (default) restricts to + // cascade bot personas; `external`/`all` extend to human same-repo PRs. const personasResult = requirePersonaIdentities(ctx.personaIdentities, prNumber, this.name); if (!personasResult.ok) return personasResult.skip; const prAuthorLogin = payload.pull_request.user.login; const gateChainSkip = - gateCascadePersona(prAuthorLogin, prNumber, personasResult.value, this.name) ?? - gateBaseBranch(payload.pull_request.base.ref, prNumber, ctx.project, this.name); + gateAuthorMode( + prAuthorLogin, + prNumber, + personasResult.value, + triggerConfig.parameters, + this.name, + ) ?? gateBaseBranch(payload.pull_request.base.ref, prNumber, ctx.project, this.name); if (gateChainSkip) return gateChainSkip; // Fetch PR details, retrying if mergeable is null (GitHub computes it asynchronously) @@ -132,6 +137,14 @@ export class PRConflictDetectedTrigger implements TriggerHandler { return skip(this.name, `PR #${prNumber} is mergeable — no conflict detected`); } + // Fork write-access skip. resolve-conflicts rebases and force-pushes — + // CASCADE cannot push to a contributor's fork. Skip a fork conflict PR + // cleanly (before bumping the attempt counter / dispatching an agent + // that would fail at push). No-op under `own` mode (cascade PRs are + // same-repo); only reachable under `external`/`all`. + const forkSkip = gateForkWriteAccess(prDetails, prNumber, this.name); + if (forkSkip) return forkSkip; + // Check attempt limit to prevent infinite loops. Side effect (PR // comment) is handler-local because the warning text differs from // other handlers and is part of the contract. diff --git a/src/triggers/github/respond-to-ci-dispatch.ts b/src/triggers/github/respond-to-ci-dispatch.ts index 48ba3afe..3ff0dcc9 100644 --- a/src/triggers/github/respond-to-ci-dispatch.ts +++ b/src/triggers/github/respond-to-ci-dispatch.ts @@ -2,7 +2,7 @@ import { type CheckSuiteStatus, githubClient } from '../../github/client.js'; import type { TriggerContext, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; import { parseRepoFullName } from '../../utils/repo.js'; -import { gateAttemptLimit } from '../shared/gates.js'; +import { gateAttemptLimit, gateForkWriteAccess } from '../shared/gates.js'; import { skip } from '../shared/skip.js'; import { checkTriggerEnabled } from '../shared/trigger-check.js'; import { @@ -28,6 +28,13 @@ export interface PRDetails { headRef: string; htmlUrl: string; title: string; + /** + * Fork write-access fields (optional). Populated from `githubClient.getPR` + * so the shared `gateForkWriteAccess` can skip fork PRs that CASCADE cannot + * push to. Optional so same-repo callers and older mocks default to non-fork. + */ + isFork?: boolean; + headRepoFullName?: string | null; } /** @@ -64,6 +71,16 @@ export async function dispatchRespondToCi(opts: { return null; } + // Fork write-access skip. respond-to-ci pushes commits — CASCADE cannot + // push to a contributor's fork. Skip BEFORE claiming the Redis dedup slot + // or bumping the attempt counter so a fork PR never consumes either. This + // is the single-sourced dispatch path for both check-suite-failure and the + // check-suite-success mixed-state fork, so one insertion covers every entry + // point. For `own`-mode dispatches `isFork` is always false (cascade PRs are + // same-repo), so this is a no-op except under `external`/`all`. + const forkSkip = gateForkWriteAccess(opts.prDetails, opts.prNumber, opts.handlerName); + if (forkSkip) return forkSkip; + const { owner, repo } = parseRepoFullName(opts.payload.repository.full_name); const headSha = opts.payload.check_suite.head_sha; diff --git a/src/triggers/github/utils.ts b/src/triggers/github/utils.ts index fb070c04..c71bbe22 100644 --- a/src/triggers/github/utils.ts +++ b/src/triggers/github/utils.ts @@ -1,59 +1,17 @@ import { lookupWorkItemForPR } from '../../db/repositories/prWorkItemsRepository.js'; -import type { PersonaIdentities } from '../../github/personas.js'; import { getPMProviderOrNull } from '../../pm/context.js'; import type { ProjectConfig } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; -export interface AuthorModeResult { - shouldTrigger: boolean; - authorMode: string; - isCascadePR: boolean; -} - -/** - * Evaluate whether a trigger should fire based on the PR author and the - * configured `authorMode` parameter. - * - * Returns `null` when personaIdentities is missing (caller should return null). - * Validates authorMode against known values and falls back to 'own'. - * - * "own" means the PR was authored by any CASCADE persona (implementer OR reviewer). - * This aligns with `isCascadeBot()` which already checks both personas. - */ -export function evaluateAuthorMode( - prAuthorLogin: string, - personaIdentities: PersonaIdentities | undefined, - parameters: Record, - handlerName: string, -): AuthorModeResult | null { - if (!personaIdentities) { - logger.info('No persona identities available, skipping', { handler: handlerName }); - return null; - } - const implLogin = personaIdentities.implementer; - const reviewerLogin = personaIdentities.reviewer; - const isImplementerPR = prAuthorLogin === implLogin || prAuthorLogin === `${implLogin}[bot]`; - const isReviewerPR = prAuthorLogin === reviewerLogin || prAuthorLogin === `${reviewerLogin}[bot]`; - const isCascadePR = isImplementerPR || isReviewerPR; - - const rawMode = parameters.authorMode; - const authorMode = - typeof rawMode === 'string' && ['own', 'external', 'all'].includes(rawMode) ? rawMode : 'own'; - - if (typeof rawMode === 'string' && authorMode !== rawMode) { - logger.warn('Invalid authorMode value, falling back to "own"', { - handler: handlerName, - configuredValue: rawMode, - }); - } - - const shouldTrigger = - authorMode === 'all' || - (authorMode === 'own' && isCascadePR) || - (authorMode === 'external' && !isCascadePR); - - return { shouldTrigger, authorMode, isCascadePR }; -} +// Re-export the author-mode evaluator from its canonical shared home so +// `pr-opened.ts` and `tests/unit/triggers/github-utils.test.ts` keep working +// untouched. The core logic now lives in `src/triggers/shared/author-mode.ts`. +export { + type AuthorMode, + type AuthorModeResult, + evaluateAuthorMode, + resolveAuthorMode, +} from '../shared/author-mode.js'; /** * Extract PR number from GitHub's refs/pull/{N}/head virtual ref. diff --git a/src/triggers/shared/author-mode.ts b/src/triggers/shared/author-mode.ts new file mode 100644 index 00000000..9089ca6f --- /dev/null +++ b/src/triggers/shared/author-mode.ts @@ -0,0 +1,79 @@ +import { isCascadeBot, type PersonaIdentities } from '../../github/personas.js'; +import { logger } from '../../utils/logging.js'; + +/** + * Single source of truth for the `authorMode` trigger parameter. + * + * Previously the author-mode logic lived in three near-duplicate copies + * (`triggers/github/utils.ts:evaluateAuthorMode`, + * `triggers/github/check-suite-decision.ts:authorModeDecision`, and the + * `pr-opened` path). This module consolidates the core so every hard-gated + * caller (review, respond-to-ci, resolve-conflicts) shares one implementation. + * + * "own" means the PR was authored by any CASCADE persona (implementer OR + * reviewer). "external" means a non-CASCADE author. "all" matches every author. + */ + +export type AuthorMode = 'own' | 'external' | 'all'; + +const VALID_AUTHOR_MODES: readonly AuthorMode[] = ['own', 'external', 'all']; + +export interface AuthorModeResult { + shouldTrigger: boolean; + authorMode: AuthorMode; + isCascadePR: boolean; +} + +/** + * Resolve the configured `authorMode` parameter to a validated value. + * + * Validates against the known set (`own` / `external` / `all`) and + * warn-and-falls-back to `own` on an unrecognised string. `own` is the safe + * default: it preserves the historical CASCADE-authored-only behavior. + */ +export function resolveAuthorMode( + parameters: Record, + handlerName?: string, +): AuthorMode { + const rawMode = parameters.authorMode; + if (typeof rawMode === 'string' && VALID_AUTHOR_MODES.includes(rawMode as AuthorMode)) { + return rawMode as AuthorMode; + } + if (typeof rawMode === 'string') { + logger.warn('Invalid authorMode value, falling back to "own"', { + handler: handlerName, + configuredValue: rawMode, + }); + } + return 'own'; +} + +/** + * Evaluate whether a trigger should fire based on the PR author and the + * configured `authorMode` parameter. + * + * Returns `null` when `personaIdentities` is missing (caller should return a + * structured skip). Validates authorMode against known values and falls back + * to `own`. + */ +export function evaluateAuthorMode( + prAuthorLogin: string, + personaIdentities: PersonaIdentities | undefined, + parameters: Record, + handlerName: string, +): AuthorModeResult | null { + if (!personaIdentities) { + logger.info('No persona identities available, skipping', { handler: handlerName }); + return null; + } + + const authorMode = resolveAuthorMode(parameters, handlerName); + const isCascadePR = isCascadeBot(prAuthorLogin, personaIdentities); + + const shouldTrigger = + authorMode === 'all' || + (authorMode === 'own' && isCascadePR) || + (authorMode === 'external' && !isCascadePR); + + return { shouldTrigger, authorMode, isCascadePR }; +} diff --git a/src/triggers/shared/gates.ts b/src/triggers/shared/gates.ts index 018465bf..32fccdd1 100644 --- a/src/triggers/shared/gates.ts +++ b/src/triggers/shared/gates.ts @@ -1,7 +1,9 @@ +import type { PRDetails } from '../../github/client.js'; import type { PersonaIdentities } from '../../github/personas.js'; import { isCascadeBot } from '../../github/personas.js'; import type { ProjectConfig, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; +import { evaluateAuthorMode } from './author-mode.js'; import { skip } from './skip.js'; /** @@ -87,6 +89,77 @@ export function gateCascadePersona( ); } +/** + * Sync gate: does the PR author match the configured `authorMode` parameter? + * + * Delegates to the shared `evaluateAuthorMode` (single source of truth for + * author-mode logic — MNG-1774). Mirrors `gateCascadePersona`'s signature so it + * slots into the `??` gate chains, but adds the `parameters` bag carrying the + * operator's `authorMode` select (own/external/all, default own). + * + * Requires a defined `PersonaIdentities` — pair with `requirePersonaIdentities` + * exactly like `gateCascadePersona`. (When `personaIdentities` is nonetheless + * undefined, `evaluateAuthorMode` returns null and this gate emits the same + * persona-resolution skip as the other gates, defense-in-depth.) + */ +export function gateAuthorMode( + prAuthorLogin: string, + prNumber: number, + personaIdentities: PersonaIdentities, + parameters: Record, + handlerName: string, +): TriggerResult | null { + const result = evaluateAuthorMode(prAuthorLogin, personaIdentities, parameters, handlerName); + if (!result) { + return skip( + handlerName, + 'Cascade persona identities could not be resolved (token / GitHub API issue)', + ); + } + if (result.shouldTrigger) return null; + logger.info(`PR author does not match configured authorMode, skipping ${handlerName}`, { + prNumber, + prAuthor: prAuthorLogin, + authorMode: result.authorMode, + isCascadePR: result.isCascadePR, + }); + return skip( + handlerName, + `PR #${prNumber} author ${prAuthorLogin} does not match configured authorMode '${result.authorMode}' (isCascadePR=${result.isCascadePR})`, + ); +} + +/** + * Sync gate: does the PR head branch live on the base repo (not a fork)? + * + * respond-to-ci and resolve-conflicts *push commits*; CASCADE has no write + * access to a contributor's fork, so a fork PR would fire → fail at push. This + * gate turns that into a clean, self-explanatory skip (MNG-1774). Only relevant + * under `authorMode: external`/`all` — cascade-authored PRs are always + * same-repo, so `isFork` is false and this is a no-op for `own` mode. + * + * `isFork` is optional on `PRDetails` and defaults to non-fork, so callers with + * older mocks / same-repo PRs pass through unchanged. + */ +export function gateForkWriteAccess( + prDetails: Pick, + prNumber: number, + handlerName: string, +): TriggerResult | null { + if (!prDetails.isFork) return null; + const forkTarget = prDetails.headRepoFullName + ? `fork ${prDetails.headRepoFullName}` + : 'a deleted/unavailable fork head'; + logger.info(`PR head branch lives on a fork, skipping ${handlerName}`, { + prNumber, + headRepoFullName: prDetails.headRepoFullName ?? null, + }); + return skip( + handlerName, + `PR #${prNumber} head branch lives on ${forkTarget} — CASCADE has no write access to push fixes; skipping ${handlerName}`, + ); +} + /** * Sync gate: is the handler's per-PR attempt counter under the limit? * diff --git a/tests/unit/github/client.test.ts b/tests/unit/github/client.test.ts index 8f4d4772..89b67635 100644 --- a/tests/unit/github/client.test.ts +++ b/tests/unit/github/client.test.ts @@ -119,6 +119,9 @@ describe('githubClient', () => { merged: false, mergeable: null, user: { login: 'test-user' }, + // No head/base repo objects in this payload → treated as non-fork. + headRepoFullName: null, + isFork: false, }); expect(mockPulls.get).toHaveBeenCalledWith({ owner: 'owner', @@ -127,6 +130,79 @@ describe('githubClient', () => { }); }); + it('detects a fork when head repo differs from base repo', async () => { + mockPulls.get.mockResolvedValue({ + data: { + number: 42, + title: 'Fork PR', + body: null, + state: 'open', + html_url: 'https://github.com/owner/repo/pull/42', + head: { + ref: 'fix/some-bug', + sha: 'forksha', + repo: { full_name: 'contributor/repo' }, + }, + base: { ref: 'main', repo: { full_name: 'owner/repo' } }, + merged: false, + user: { login: 'contributor' }, + }, + }); + + const result = await withGitHubToken('test-token', () => + githubClient.getPR('owner', 'repo', 42), + ); + + expect(result.isFork).toBe(true); + expect(result.headRepoFullName).toBe('contributor/repo'); + }); + + it('treats same-repo PR (head.full_name === base.full_name) as non-fork', async () => { + mockPulls.get.mockResolvedValue({ + data: { + number: 42, + title: 'Same-repo PR', + body: null, + state: 'open', + html_url: 'https://github.com/owner/repo/pull/42', + head: { ref: 'feature/x', sha: 'abc', repo: { full_name: 'owner/repo' } }, + base: { ref: 'main', repo: { full_name: 'owner/repo' } }, + merged: false, + user: { login: 'cascade-impl' }, + }, + }); + + const result = await withGitHubToken('test-token', () => + githubClient.getPR('owner', 'repo', 42), + ); + + expect(result.isFork).toBe(false); + expect(result.headRepoFullName).toBe('owner/repo'); + }); + + it('treats a deleted fork head (head.repo === null) as a fork', async () => { + mockPulls.get.mockResolvedValue({ + data: { + number: 42, + title: 'Deleted fork PR', + body: null, + state: 'open', + html_url: 'https://github.com/owner/repo/pull/42', + head: { ref: 'fix/gone', sha: 'gonesha', repo: null }, + base: { ref: 'main', repo: { full_name: 'owner/repo' } }, + merged: false, + user: { login: 'contributor' }, + }, + }); + + const result = await withGitHubToken('test-token', () => + githubClient.getPR('owner', 'repo', 42), + ); + + expect(result.isFork).toBe(true); + expect(result.headRepoFullName).toBeNull(); + }); + it('handles null merged field and missing user', async () => { mockPulls.get.mockResolvedValue({ data: { diff --git a/tests/unit/triggers/check-suite-failure.test.ts b/tests/unit/triggers/check-suite-failure.test.ts index 67103e80..4f5f27af 100644 --- a/tests/unit/triggers/check-suite-failure.test.ts +++ b/tests/unit/triggers/check-suite-failure.test.ts @@ -37,7 +37,7 @@ vi.mock('../../../src/db/repositories/prWorkItemsRepository.js', () => ({ })); import { lookupWorkItemForPR } from '../../../src/db/repositories/prWorkItemsRepository.js'; -import { checkTriggerEnabled } from '../../../src/triggers/shared/trigger-check.js'; +import { checkTriggerEnabledWithParams } from '../../../src/triggers/shared/trigger-check.js'; import { expectSkipFor } from '../../helpers/triggerAssertions.js'; @@ -191,7 +191,11 @@ describe('CheckSuiteFailureTrigger', () => { it('returns null when trigger is disabled (so the registry can try the next matcher)', async () => { // Disabled-at-config returns null, not a structured skip — see // `checkTriggerEnablement` contract for the shadowing-bug context. - vi.mocked(checkTriggerEnabled).mockResolvedValueOnce(false); + // The handler now resolves enablement + authorMode params in one call. + vi.mocked(checkTriggerEnabledWithParams).mockResolvedValueOnce({ + enabled: false, + parameters: {}, + }); const ctx: TriggerContext = { project: mockProject, @@ -202,7 +206,7 @@ describe('CheckSuiteFailureTrigger', () => { const result = await trigger.handle(ctx); expect(result).toBeNull(); - expect(checkTriggerEnabled).toHaveBeenCalledWith( + expect(checkTriggerEnabledWithParams).toHaveBeenCalledWith( 'test', 'respond-to-ci', 'scm:check-suite-failure', @@ -292,7 +296,12 @@ describe('CheckSuiteFailureTrigger', () => { const result = await trigger.handle(ctx); - expectSkip(result, /not authored by a cascade persona.*author: some-human/i); + // Default authorMode 'own' filters human-authored PRs with the + // author-mode skip message (before the base-branch gate). + expectSkip( + result, + /author some-human does not match configured authorMode 'own' \(isCascadePR=false\)/i, + ); expect(githubClient.getCheckSuiteStatus).not.toHaveBeenCalled(); }); @@ -352,7 +361,11 @@ describe('CheckSuiteFailureTrigger', () => { const result = await trigger.handle(ctx); - expectSkip(result, /not authored by a cascade persona.*author: some-human/); + // Default authorMode 'own' → human author skipped via author-mode gate. + expectSkip( + result, + /author some-human does not match configured authorMode 'own' \(isCascadePR=false\)/, + ); }); // Fix 3: gate widening — both implementer AND reviewer personas should @@ -791,5 +804,153 @@ describe('CheckSuiteFailureTrigger', () => { expect(result).not.toBeNull(); expect(result?.agentType).toBe('respond-to-ci'); }); + + // MNG-1774: authorMode extension + fork write-access skip. + describe('authorMode + fork write-access (MNG-1774)', () => { + it("dispatches respond-to-ci on a HUMAN same-repo PR under authorMode 'all'", async () => { + vi.mocked(checkTriggerEnabledWithParams).mockResolvedValueOnce({ + enabled: true, + parameters: { authorMode: 'all' }, + }); + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 42, + title: 'Human PR', + body: null, + state: 'open', + htmlUrl: 'https://github.com/owner/repo/pull/42', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: false, + user: { login: 'some-human' }, + isFork: false, + }); + vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ + allPassing: false, + totalCount: 1, + checkRuns: [{ name: 'test', status: 'completed', conclusion: 'failure' }], + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeFailurePayload(), + personaIdentities: mockPersonaIdentities, + }; + + const result = await trigger.handle(ctx); + + expect(result?.agentType).toBe('respond-to-ci'); + expect(result?.prNumber).toBe(42); + }); + + it("dispatches respond-to-ci on a HUMAN same-repo PR under authorMode 'external'", async () => { + vi.mocked(checkTriggerEnabledWithParams).mockResolvedValueOnce({ + enabled: true, + parameters: { authorMode: 'external' }, + }); + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 42, + title: 'Human PR', + body: null, + state: 'open', + htmlUrl: 'https://github.com/owner/repo/pull/42', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: false, + user: { login: 'some-human' }, + isFork: false, + }); + vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ + allPassing: false, + totalCount: 1, + checkRuns: [{ name: 'test', status: 'completed', conclusion: 'failure' }], + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeFailurePayload(), + personaIdentities: mockPersonaIdentities, + }; + + const result = await trigger.handle(ctx); + + expect(result?.agentType).toBe('respond-to-ci'); + }); + + it("skips a CASCADE-authored PR under authorMode 'external'", async () => { + vi.mocked(checkTriggerEnabledWithParams).mockResolvedValueOnce({ + enabled: true, + parameters: { authorMode: 'external' }, + }); + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 42, + title: 'Cascade PR', + body: null, + state: 'open', + htmlUrl: 'https://github.com/owner/repo/pull/42', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: false, + user: { login: 'cascade-impl' }, + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeFailurePayload(), + personaIdentities: mockPersonaIdentities, + }; + + const result = await trigger.handle(ctx); + + expectSkip( + result, + /author cascade-impl does not match configured authorMode 'external' \(isCascadePR=true\)/, + ); + expect(githubClient.getCheckSuiteStatus).not.toHaveBeenCalled(); + }); + + it('skips a FORK PR with an explicit fork write-access reason (no dispatch)', async () => { + vi.mocked(checkTriggerEnabledWithParams).mockResolvedValueOnce({ + enabled: true, + parameters: { authorMode: 'all' }, + }); + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 42, + title: 'Fork PR', + body: null, + state: 'open', + htmlUrl: 'https://github.com/owner/repo/pull/42', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: false, + user: { login: 'some-human' }, + isFork: true, + headRepoFullName: 'contributor/repo', + }); + vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ + allPassing: false, + totalCount: 1, + checkRuns: [{ name: 'test', status: 'completed', conclusion: 'failure' }], + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeFailurePayload(), + personaIdentities: mockPersonaIdentities, + }; + + const result = await trigger.handle(ctx); + + expectSkip(result, /head branch lives on fork contributor\/repo.*no write access/i); + expect(result?.agentType).toBeNull(); + }); + }); }); }); diff --git a/tests/unit/triggers/github/check-suite-decision.test.ts b/tests/unit/triggers/github/check-suite-decision.test.ts index 7d72d50a..e3ecb55d 100644 --- a/tests/unit/triggers/github/check-suite-decision.test.ts +++ b/tests/unit/triggers/github/check-suite-decision.test.ts @@ -131,6 +131,84 @@ describe('decideCheckSuiteOutcome', () => { expect(decision).toBeNull(); }); + // MNG-1774: respond-to-ci mode now carries authorMode parameters and routes + // through the same shared author-mode evaluator as review mode. + describe('respond-to-ci mode (MNG-1774)', () => { + const respondBase = { + ...baseOptions, + handlerName: 'check-suite-failure', + } as const; + + it("skips a cascade-authored PR under respond-to-ci authorMode 'external'", () => { + const decision = decideCheckSuiteGates({ + ...respondBase, + prAuthorLogin: 'cascade-impl', + mode: { kind: 'respond-to-ci', parameters: { authorMode: 'external' } }, + }); + + expect(decision).toEqual({ + action: 'skip', + message: + "PR #42 author cascade-impl does not match configured authorMode 'external' (isCascadePR=true)", + }); + }); + + it("passes a human-authored PR under respond-to-ci authorMode 'all'", () => { + const decision = decideCheckSuiteGates({ + ...respondBase, + prAuthorLogin: 'random-contributor', + mode: { kind: 'respond-to-ci', parameters: { authorMode: 'all' } }, + }); + + expect(decision).toBeNull(); + }); + + it("skips a human-authored PR under respond-to-ci authorMode 'own' (default)", () => { + const decision = decideCheckSuiteGates({ + ...respondBase, + prAuthorLogin: 'random-contributor', + mode: { kind: 'respond-to-ci', parameters: {} }, + }); + + expect(decision).toEqual({ + action: 'skip', + message: + "PR #42 author random-contributor does not match configured authorMode 'own' (isCascadePR=false)", + }); + }); + + it('still returns the "all passed — no action" skip for respond-to-ci when every check passes', () => { + const decision = decideCheckSuiteOutcome({ + ...respondBase, + prAuthorLogin: 'cascade-impl', + mode: { kind: 'respond-to-ci', parameters: {} }, + checkStatus: status([ + { name: 'lint', status: 'completed', conclusion: 'success' }, + { name: 'test', status: 'completed', conclusion: 'success' }, + ]), + }); + + expect(decision).toEqual({ + action: 'skip', + message: 'All 2 checks passed for PR #42 — no action needed', + }); + }); + + it('returns respond-to-ci for a mixed aggregate under respond-to-ci mode', () => { + const decision = decideCheckSuiteOutcome({ + ...respondBase, + prAuthorLogin: 'cascade-impl', + mode: { kind: 'respond-to-ci', parameters: {} }, + checkStatus: status([ + { name: 'lint', status: 'completed', conclusion: 'success' }, + { name: 'test', status: 'completed', conclusion: 'failure' }, + ]), + }); + + expect(decision).toEqual({ action: 'respond-to-ci' }); + }); + }); + it('rejects cascade-authored stacked PR if persona identities cannot be resolved', () => { // Defense in depth: with no personaIdentities, isCascadeBot is unreliable. // The author-mode gate already returns its own skip in that case (see diff --git a/tests/unit/triggers/pr-conflict-detected.test.ts b/tests/unit/triggers/pr-conflict-detected.test.ts index 66eb7251..dd14190b 100644 --- a/tests/unit/triggers/pr-conflict-detected.test.ts +++ b/tests/unit/triggers/pr-conflict-detected.test.ts @@ -21,7 +21,7 @@ import { PRConflictDetectedTrigger, resetConflictAttempts, } from '../../../src/triggers/github/pr-conflict-detected.js'; -import { checkTriggerEnabled } from '../../../src/triggers/shared/trigger-check.js'; +import { checkTriggerEnabledWithParams } from '../../../src/triggers/shared/trigger-check.js'; import type { TriggerContext } from '../../../src/triggers/types.js'; import { createMockProject } from '../../helpers/factories.js'; import { mockPersonaIdentities } from '../../helpers/mockPersonas.js'; @@ -126,7 +126,10 @@ describe('PRConflictDetectedTrigger', () => { // so the registry's first-match loop continues to the next // matcher. See `src/triggers/shared/trigger-check.ts` for the // disabled-shadowing contract. - vi.mocked(checkTriggerEnabled).mockResolvedValueOnce(false); + vi.mocked(checkTriggerEnabledWithParams).mockResolvedValueOnce({ + enabled: false, + parameters: {}, + }); const ctx: TriggerContext = { project: mockProject, @@ -137,7 +140,7 @@ describe('PRConflictDetectedTrigger', () => { const result = await trigger.handle(ctx); expect(result).toBeNull(); - expect(checkTriggerEnabled).toHaveBeenCalledWith( + expect(checkTriggerEnabledWithParams).toHaveBeenCalledWith( 'test', 'resolve-conflicts', 'scm:pr-conflict-detected', @@ -585,5 +588,110 @@ describe('PRConflictDetectedTrigger', () => { expect(result).not.toBeNull(); expect(result?.agentType).toBe('resolve-conflicts'); }); + + // MNG-1774: authorMode extension + fork write-access skip. + describe('authorMode + fork write-access (MNG-1774)', () => { + const makeHumanPayload = (login = 'some-human') => + makeSynchronizePayload({ + pull_request: { + ...makeSynchronizePayload().pull_request, + user: { login }, + }, + }); + + it("dispatches resolve-conflicts on a HUMAN same-repo PR under authorMode 'all'", async () => { + vi.mocked(checkTriggerEnabledWithParams).mockResolvedValueOnce({ + enabled: true, + parameters: { authorMode: 'all' }, + }); + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 42, + title: 'Human PR', + body: null, + state: 'open', + htmlUrl: 'https://github.com/owner/repo/pull/42', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: false, + mergeable: false, + user: { login: 'some-human' }, + isFork: false, + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeHumanPayload(), + personaIdentities: mockPersonaIdentities, + }; + + const result = await trigger.handle(ctx); + + expect(result?.agentType).toBe('resolve-conflicts'); + expect(result?.prNumber).toBe(42); + }); + + it("skips a CASCADE-authored PR under authorMode 'external'", async () => { + vi.mocked(checkTriggerEnabledWithParams).mockResolvedValueOnce({ + enabled: true, + parameters: { authorMode: 'external' }, + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeSynchronizePayload(), + personaIdentities: mockPersonaIdentities, + }; + + const result = await trigger.handle(ctx); + + expect(result?.agentType).toBeNull(); + expect(result?.skipReason?.message).toMatch( + /author cascade-impl does not match configured authorMode 'external' \(isCascadePR=true\)/, + ); + // Gate runs before the PR fetch — no mergeability lookup happened. + expect(githubClient.getPR).not.toHaveBeenCalled(); + }); + + it('skips a FORK conflict PR with an explicit fork write-access reason (no dispatch)', async () => { + vi.mocked(checkTriggerEnabledWithParams).mockResolvedValueOnce({ + enabled: true, + parameters: { authorMode: 'all' }, + }); + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 42, + title: 'Fork PR', + body: null, + state: 'open', + htmlUrl: 'https://github.com/owner/repo/pull/42', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: false, + mergeable: false, + user: { login: 'some-human' }, + isFork: true, + headRepoFullName: 'contributor/repo', + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeHumanPayload(), + personaIdentities: mockPersonaIdentities, + }; + + const result = await trigger.handle(ctx); + + expect(result?.agentType).toBeNull(); + expect(result?.skipReason?.message).toMatch( + /head branch lives on fork contributor\/repo.*no write access/i, + ); + // Fork skip happens before the attempt counter increments / dispatch. + expect(githubClient.createPRComment).not.toHaveBeenCalled(); + }); + }); }); }); diff --git a/tests/unit/triggers/shared/author-mode.test.ts b/tests/unit/triggers/shared/author-mode.test.ts new file mode 100644 index 00000000..15685d24 --- /dev/null +++ b/tests/unit/triggers/shared/author-mode.test.ts @@ -0,0 +1,61 @@ +import { describe, expect, it } from 'vitest'; + +import type { PersonaIdentities } from '../../../../src/github/personas.js'; +import { + type AuthorMode, + evaluateAuthorMode, + resolveAuthorMode, +} from '../../../../src/triggers/shared/author-mode.js'; + +const personas: PersonaIdentities = { + implementer: 'cascade-impl', + reviewer: 'cascade-rev', +}; + +describe('resolveAuthorMode', () => { + it.each<[Record, AuthorMode]>([ + [{ authorMode: 'own' }, 'own'], + [{ authorMode: 'external' }, 'external'], + [{ authorMode: 'all' }, 'all'], + ])('returns the validated value for %o', (parameters, expected) => { + expect(resolveAuthorMode(parameters)).toBe(expected); + }); + + it('falls back to own when authorMode is absent', () => { + expect(resolveAuthorMode({})).toBe('own'); + }); + + it('falls back to own on an unrecognised value', () => { + expect(resolveAuthorMode({ authorMode: 'bogus' })).toBe('own'); + }); +}); + +describe('evaluateAuthorMode', () => { + it('returns null when personaIdentities is missing', () => { + expect(evaluateAuthorMode('anyone', undefined, {}, 'h')).toBeNull(); + }); + + // own/external/all × cascade-impl / cascade-reviewer / human matrix. + it.each<[AuthorMode, string, boolean]>([ + ['own', 'cascade-impl', true], + ['own', 'cascade-rev', true], + ['own', 'some-human', false], + ['external', 'cascade-impl', false], + ['external', 'cascade-rev', false], + ['external', 'some-human', true], + ['all', 'cascade-impl', true], + ['all', 'cascade-rev', true], + ['all', 'some-human', true], + ])('authorMode %s + author %s → shouldTrigger=%s', (authorMode, login, shouldTrigger) => { + const result = evaluateAuthorMode(login, personas, { authorMode }, 'h'); + expect(result).not.toBeNull(); + expect(result?.shouldTrigger).toBe(shouldTrigger); + expect(result?.authorMode).toBe(authorMode); + }); + + it('recognises the [bot]-suffixed persona variants as cascade PRs', () => { + const result = evaluateAuthorMode('cascade-impl[bot]', personas, { authorMode: 'own' }, 'h'); + expect(result?.isCascadePR).toBe(true); + expect(result?.shouldTrigger).toBe(true); + }); +}); diff --git a/tests/unit/triggers/shared/gates.test.ts b/tests/unit/triggers/shared/gates.test.ts index c510c9df..a848a6bd 100644 --- a/tests/unit/triggers/shared/gates.test.ts +++ b/tests/unit/triggers/shared/gates.test.ts @@ -3,8 +3,10 @@ import { describe, expect, it } from 'vitest'; import type { PersonaIdentities } from '../../../../src/github/personas.js'; import { gateAttemptLimit, + gateAuthorMode, gateBaseBranch, gateCascadePersona, + gateForkWriteAccess, requirePersonaIdentities, } from '../../../../src/triggers/shared/gates.js'; import type { ProjectConfig } from '../../../../src/types/index.js'; @@ -62,6 +64,100 @@ describe('gateCascadePersona', () => { }); }); +describe('gateAuthorMode', () => { + // authorMode 'own' (default): only cascade personas pass. + it("passes cascade authors and skips humans under authorMode 'own'", () => { + expect( + gateAuthorMode('cascade-impl', 42, mockPersonas, { authorMode: 'own' }, 'respond-to-ci'), + ).toBeNull(); + expect( + gateAuthorMode('cascade-rev', 42, mockPersonas, { authorMode: 'own' }, 'respond-to-ci'), + ).toBeNull(); + + const humanSkip = gateAuthorMode( + 'some-human', + 42, + mockPersonas, + { authorMode: 'own' }, + 'respond-to-ci', + ); + expect(humanSkip?.skipReason?.handler).toBe('respond-to-ci'); + expect(humanSkip?.skipReason?.message).toMatch( + /author some-human does not match configured authorMode 'own' \(isCascadePR=false\)/, + ); + }); + + // authorMode 'external': only non-cascade authors pass. + it("passes humans and skips cascade authors under authorMode 'external'", () => { + expect( + gateAuthorMode( + 'some-human', + 42, + mockPersonas, + { authorMode: 'external' }, + 'resolve-conflicts', + ), + ).toBeNull(); + + const cascadeSkip = gateAuthorMode( + 'cascade-impl', + 42, + mockPersonas, + { authorMode: 'external' }, + 'resolve-conflicts', + ); + expect(cascadeSkip?.skipReason?.message).toMatch( + /author cascade-impl does not match configured authorMode 'external' \(isCascadePR=true\)/, + ); + }); + + // authorMode 'all': every author passes. + it("passes both cascade and human authors under authorMode 'all'", () => { + expect(gateAuthorMode('cascade-impl', 42, mockPersonas, { authorMode: 'all' }, 'h')).toBeNull(); + expect(gateAuthorMode('some-human', 42, mockPersonas, { authorMode: 'all' }, 'h')).toBeNull(); + }); + + // Missing/invalid authorMode falls back to 'own'. + it('defaults to own when authorMode is absent', () => { + expect(gateAuthorMode('cascade-impl', 42, mockPersonas, {}, 'h')).toBeNull(); + const humanSkip = gateAuthorMode('some-human', 42, mockPersonas, {}, 'h'); + expect(humanSkip?.skipReason?.message).toMatch(/authorMode 'own'/); + }); +}); + +describe('gateForkWriteAccess', () => { + it('returns null for a same-repo (non-fork) PR', () => { + expect( + gateForkWriteAccess({ isFork: false, headRepoFullName: 'owner/repo' }, 42, 'respond-to-ci'), + ).toBeNull(); + }); + + it('returns null when isFork is undefined (older mocks default to non-fork)', () => { + expect(gateForkWriteAccess({}, 42, 'respond-to-ci')).toBeNull(); + }); + + it('returns a structured skip naming the fork head repo', () => { + const result = gateForkWriteAccess( + { isFork: true, headRepoFullName: 'contributor/repo' }, + 42, + 'respond-to-ci', + ); + expect(result?.skipReason?.handler).toBe('respond-to-ci'); + expect(result?.skipReason?.message).toMatch( + /PR #42 head branch lives on fork contributor\/repo.*no write access.*respond-to-ci/i, + ); + }); + + it('returns a skip with a deleted-fork phrasing when headRepoFullName is null', () => { + const result = gateForkWriteAccess( + { isFork: true, headRepoFullName: null }, + 42, + 'resolve-conflicts', + ); + expect(result?.skipReason?.message).toMatch(/deleted\/unavailable fork head/i); + }); +}); + describe('gateAttemptLimit', () => { it('returns null when attempts is below the limit', () => { expect(gateAttemptLimit(0, 3, 42, 'h')).toBeNull();