From ac765bfd9eba011f7e0a60e2cd8e78f2dae5472b Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Fri, 10 Jul 2026 16:04:04 +0200 Subject: [PATCH 1/4] fix(review): add policy-file fallback so comment-only policy reaches cascade-tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The claude subprocess chain (@anthropic-ai/claude-code ≤ 2.1.185, bun-compiled) does not forward all custom env vars to bash subprocesses. CASCADE_REVIEW_EVENT_POLICY is only injected into the SDK env dict, never into the Docker container env — so it is uniquely vulnerable to this filtering (GITHUB_TOKEN and other credentials are also in the container env via buildWorkerEnvWithProjectId and survive). Fix: buildExecutionPlan writes the policy to /tmp/cascade-review-event-policy (inside the ephemeral worker container) before the agent starts. resolveEventPolicyFromEnv in cascade-tools scm create-pr-review reads this file as a fallback when the env var is absent. Both mechanisms remain active — whichever survives the subprocess chain wins. Co-Authored-By: Claude Sonnet 4.6 Claude-Session: https://claude.ai/code/session_011ZaMpedeE8PXsix84cWsq1 --- src/backends/secretOrchestrator.ts | 19 +++- src/cli/scm/create-pr-review.ts | 32 +++++- src/config/reviewEventPolicy.ts | 10 ++ tests/unit/cli/scm/create-pr-review.test.ts | 120 +++++++++++++++++++- 4 files changed, 175 insertions(+), 6 deletions(-) diff --git a/src/backends/secretOrchestrator.ts b/src/backends/secretOrchestrator.ts index f5c832162..59968a252 100644 --- a/src/backends/secretOrchestrator.ts +++ b/src/backends/secretOrchestrator.ts @@ -1,3 +1,4 @@ +import { writeFileSync } from 'node:fs'; import { createIntegrationChecker, resolveEffectiveCapabilities, @@ -12,7 +13,11 @@ import { resolveModelConfig } from '../agents/shared/modelResolution.js'; import { buildPromptContext } from '../agents/shared/promptContext.js'; import type { createAgentLogger } from '../agents/utils/logging.js'; import { mergeEngineSettings } from '../config/engineSettings.js'; -import { isCommentOnlyReview, resolveReviewEventPolicy } from '../config/reviewEventPolicy.js'; +import { + isCommentOnlyReview, + REVIEW_EVENT_POLICY_FILE, + resolveReviewEventPolicy, +} from '../config/reviewEventPolicy.js'; import { filterPostingGadgetNames, resolveUpdateChannel } from '../config/updateChannel.js'; import { loadPartials } from '../db/repositories/partialsRepository.js'; import { withGitHubToken } from '../github/client.js'; @@ -148,6 +153,18 @@ export async function buildExecutionPlan( // Build per-project secrets with CASCADE env var injections const projectSecrets = await augmentProjectSecrets(project, agentType, input); + // Write the review event policy to a well-known file so cascade-tools can read + // it even when the env var is stripped by the claude subprocess chain. + // (@anthropic-ai/claude-code ≤ 2.1.185 does not forward all custom env vars to + // bash subprocesses; the file lives in the container's ephemeral /tmp.) + if (isCommentOnlyReview(resolveReviewEventPolicy(project, agentType))) { + try { + writeFileSync(REVIEW_EVENT_POLICY_FILE, 'comment-only', 'utf-8'); + } catch { + // Non-fatal — env-var mechanism remains the primary path + } + } + // Inject pre-seeded progress comment ID so the subprocess finds it at startup injectProgressCommentId( projectSecrets, diff --git a/src/cli/scm/create-pr-review.ts b/src/cli/scm/create-pr-review.ts index bc8902075..2e998e117 100644 --- a/src/cli/scm/create-pr-review.ts +++ b/src/cli/scm/create-pr-review.ts @@ -1,8 +1,11 @@ +import { readFileSync } from 'node:fs'; import { GITHUB_ACK_COMMENT_ID_ENV_VAR } from '../../backends/secretBuilder.js'; import { DEFAULT_REVIEW_EVENT_POLICY, REVIEW_EVENT_POLICY_ENV_VAR, + REVIEW_EVENT_POLICY_FILE, type ReviewEvent, + type ReviewEventPolicy, ReviewEventPolicySchema, } from '../../config/reviewEventPolicy.js'; import { createPRReview } from '../../gadgets/github/core/createPRReview.js'; @@ -31,10 +34,31 @@ async function deleteAckComment(owner: string, repo: string): Promise { } } -/** Resolve the review event policy injected by the router (absent/invalid → `all`). */ -function resolveEventPolicyFromEnv() { - const parsed = ReviewEventPolicySchema.safeParse(process.env[REVIEW_EVENT_POLICY_ENV_VAR]); - return parsed.success ? parsed.data : DEFAULT_REVIEW_EVENT_POLICY; +/** + * Resolve the review event policy for this run. + * + * Checks two sources in order: + * 1. `CASCADE_REVIEW_EVENT_POLICY` env var — injected into the SDK env dict by + * `augmentProjectSecrets`. May be stripped by the claude subprocess chain. + * 2. Policy file written to `/tmp/cascade-review-event-policy` by the worker + * process before the agent starts — survives subprocess env filtering. + * + * Absent/invalid in both sources → {@link DEFAULT_REVIEW_EVENT_POLICY}. + */ +function resolveEventPolicyFromEnv(): ReviewEventPolicy { + const envParsed = ReviewEventPolicySchema.safeParse(process.env[REVIEW_EVENT_POLICY_ENV_VAR]); + if (envParsed.success) return envParsed.data; + + try { + const fileParsed = ReviewEventPolicySchema.safeParse( + readFileSync(REVIEW_EVENT_POLICY_FILE, 'utf-8').trim(), + ); + if (fileParsed.success) return fileParsed.data; + } catch { + // File absent or unreadable → default + } + + return DEFAULT_REVIEW_EVENT_POLICY; } export default createCLICommand(createPRReviewDef, async (params) => { diff --git a/src/config/reviewEventPolicy.ts b/src/config/reviewEventPolicy.ts index 05204aa39..55b4c2269 100644 --- a/src/config/reviewEventPolicy.ts +++ b/src/config/reviewEventPolicy.ts @@ -42,6 +42,16 @@ export const ReviewEventPolicySchema = z.enum(REVIEW_EVENT_POLICIES); */ export const REVIEW_EVENT_POLICY_ENV_VAR = 'CASCADE_REVIEW_EVENT_POLICY'; +/** + * Policy file written by the worker process before the agent starts. Used as a + * fallback when {@link REVIEW_EVENT_POLICY_ENV_VAR} is stripped by the claude + * subprocess chain (observed with @anthropic-ai/claude-code ≤ 2.1.185 — the + * bun-compiled binary does not forward all custom env vars to bash subprocesses). + * The path is fixed inside the ephemeral worker container's /tmp, so there is no + * cross-run collision. + */ +export const REVIEW_EVENT_POLICY_FILE = '/tmp/cascade-review-event-policy'; + /** The GitHub pull-request review event types CreatePRReview can submit. */ export const REVIEW_EVENTS = ['APPROVE', 'REQUEST_CHANGES', 'COMMENT'] as const; diff --git a/tests/unit/cli/scm/create-pr-review.test.ts b/tests/unit/cli/scm/create-pr-review.test.ts index d279bdd32..e485fd176 100644 --- a/tests/unit/cli/scm/create-pr-review.test.ts +++ b/tests/unit/cli/scm/create-pr-review.test.ts @@ -1,9 +1,11 @@ -import { existsSync, readFileSync, rmSync } from 'node:fs'; +import { existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { REVIEW_EVENT_POLICY_FILE } from '../../../../src/config/reviewEventPolicy.js'; + // Mock createPRReview before importing the command const mockCreatePRReview = vi.fn(); vi.mock('../../../../src/gadgets/github/core/createPRReview.js', () => ({ @@ -200,3 +202,119 @@ describe('CreatePRReviewCommand — GitHub ack comment deletion', () => { expect(mockDeletePRComment).not.toHaveBeenCalled(); }); }); + +describe('CreatePRReviewCommand — review event policy resolution', () => { + let originalPolicyEnv: string | undefined; + let originalSidecarEnv: string | undefined; + let sidecarPath: string; + + beforeEach(() => { + originalPolicyEnv = process.env.CASCADE_REVIEW_EVENT_POLICY; + originalSidecarEnv = process.env.CASCADE_REVIEW_SIDECAR_PATH; + sidecarPath = join(tmpdir(), `cascade-test-review-policy-${Date.now()}.json`); + process.env.CASCADE_REVIEW_SIDECAR_PATH = sidecarPath; + Reflect.deleteProperty(process.env, 'CASCADE_REVIEW_EVENT_POLICY'); + // Clean up any leftover policy file from previous tests + try { + rmSync(REVIEW_EVENT_POLICY_FILE, { force: true }); + } catch { + /* no-op */ + } + mockCreatePRReview.mockResolvedValue({ + reviewUrl: 'https://github.com/owner/repo/pull/1#pullrequestreview-1', + }); + }); + + afterEach(() => { + try { + rmSync(sidecarPath, { force: true }); + } catch { + /* no-op */ + } + try { + rmSync(REVIEW_EVENT_POLICY_FILE, { force: true }); + } catch { + /* no-op */ + } + if (originalPolicyEnv !== undefined) { + process.env.CASCADE_REVIEW_EVENT_POLICY = originalPolicyEnv; + } else { + Reflect.deleteProperty(process.env, 'CASCADE_REVIEW_EVENT_POLICY'); + } + if (originalSidecarEnv !== undefined) { + process.env.CASCADE_REVIEW_SIDECAR_PATH = originalSidecarEnv; + } else { + Reflect.deleteProperty(process.env, 'CASCADE_REVIEW_SIDECAR_PATH'); + } + vi.restoreAllMocks(); + }); + + it('uses env var when present', async () => { + process.env.CASCADE_REVIEW_EVENT_POLICY = 'comment-only'; + + const cmd = new CreatePRReviewCommand([], {} as never); + vi.mocked(cmd.parse).mockResolvedValue(makeParseResult({ event: 'REQUEST_CHANGES' })); + + await cmd.execute(); + + expect(mockCreatePRReview).toHaveBeenCalledWith( + expect.any(Object), + expect.objectContaining({ eventPolicy: 'comment-only' }), + ); + }); + + it('falls back to policy file when env var is absent', async () => { + writeFileSync(REVIEW_EVENT_POLICY_FILE, 'comment-only', 'utf-8'); + + const cmd = new CreatePRReviewCommand([], {} as never); + vi.mocked(cmd.parse).mockResolvedValue(makeParseResult({ event: 'REQUEST_CHANGES' })); + + await cmd.execute(); + + expect(mockCreatePRReview).toHaveBeenCalledWith( + expect.any(Object), + expect.objectContaining({ eventPolicy: 'comment-only' }), + ); + }); + + it('env var wins over policy file when both are present', async () => { + process.env.CASCADE_REVIEW_EVENT_POLICY = 'all'; + writeFileSync(REVIEW_EVENT_POLICY_FILE, 'comment-only', 'utf-8'); + + const cmd = new CreatePRReviewCommand([], {} as never); + vi.mocked(cmd.parse).mockResolvedValue(makeParseResult({ event: 'REQUEST_CHANGES' })); + + await cmd.execute(); + + expect(mockCreatePRReview).toHaveBeenCalledWith( + expect.any(Object), + expect.objectContaining({ eventPolicy: 'all' }), + ); + }); + + it('defaults to all when both env var and file are absent', async () => { + const cmd = new CreatePRReviewCommand([], {} as never); + vi.mocked(cmd.parse).mockResolvedValue(makeParseResult({ event: 'REQUEST_CHANGES' })); + + await cmd.execute(); + + expect(mockCreatePRReview).toHaveBeenCalledWith( + expect.any(Object), + expect.objectContaining({ eventPolicy: 'all' }), + ); + }); + + it('defaults to all when policy file contains invalid content', async () => { + writeFileSync(REVIEW_EVENT_POLICY_FILE, 'not-a-valid-policy', 'utf-8'); + + const cmd = new CreatePRReviewCommand([], {} as never); + vi.mocked(cmd.parse).mockResolvedValue(makeParseResult({ event: 'REQUEST_CHANGES' })); + + await cmd.execute(); + + expect(mockCreatePRReview).toHaveBeenCalledWith( + expect.any(Object), + expect.objectContaining({ eventPolicy: 'all' }), + ); + }); +}); From 4b55ff4e70c152377bbf5e366d5c5c6d7148f2d4 Mon Sep 17 00:00:00 2001 From: aaight Date: Fri, 10 Jul 2026 17:08:42 +0200 Subject: [PATCH 2/4] fix(triggers): dispatch review for self-directed review requests (MNG-1746) (#1480) Co-authored-by: Cascade Bot --- CLAUDE.md | 2 +- docs/architecture/03-trigger-system.md | 2 + src/triggers/github/review-requested.ts | 53 ++++++++++++++++-- tests/unit/triggers/review-requested.test.ts | 58 ++++++++++++++++++-- 4 files changed, 104 insertions(+), 11 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 8c0c0ca75..a034bb9d0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -102,7 +102,7 @@ cascade projects credentials-set --key GITHUB_TOKEN_REVIEWER --value ghp_.. - `respond-to-review` fires **only** when the **reviewer** persona submits `changes_requested`. - `respond-to-pr-comment` skips @mentions from **any** known persona. - `check-suite-success` checks reviews from the **reviewer** persona specifically. -- All trigger handlers use `isCascadeBot(login)` to filter self-events. +- All trigger handlers use `isCascadeBot(login)` to filter self-events. **Self-directed exemption:** `review-requested` treats a request where `sender === requested_reviewer` (both a CASCADE persona) as human-initiated and dispatches `review` — the shared-`GITHUB_TOKEN_REVIEWER` contributor re-requesting their own review. Cross-persona `review_requested` events (`sender ≠ requested_reviewer`, e.g. an implementer-authored PR auto-assigning the reviewer) still skip. Safe because CASCADE never programmatically calls GitHub's "request reviewers" API and review *submissions* emit `pull_request_review`, not `review_requested`. ## Agent update channel diff --git a/docs/architecture/03-trigger-system.md b/docs/architecture/03-trigger-system.md index d646708ea..f5d6900f6 100644 --- a/docs/architecture/03-trigger-system.md +++ b/docs/architecture/03-trigger-system.md @@ -171,6 +171,8 @@ Bare `null` means "this handler did not handle the event; continue registry disp The router preserves structured skips in webhook logs with `Trigger skipped: `. Use structured skip for disabled trigger config, author-mode gates, self-loop gates, incomplete aggregate check-suite state, missing PR/work-item prerequisites, and similar expected non-dispatch outcomes. +Self-loop gates are intentionally narrow. `ReviewRequestedTrigger` exempts a **self-directed** request (`sender === requested_reviewer`, both a CASCADE persona) and dispatches `review`, because a human using the shared `GITHUB_TOKEN_REVIEWER` can re-request their own review; only cross-persona `review_requested` events (`sender ≠ requested_reviewer`) are skipped as bot loops. This is safe because CASCADE never programmatically calls GitHub's "request reviewers" API and review *submissions* emit `pull_request_review`, not `review_requested`. + ### Deferred re-checks Handlers that cannot make a final decision yet can return `deferredRecheck: { delayMs, coalesceKey, recheckKind? }` with `agentType: null`. The router schedules a coalesced delayed BullMQ job and exits without spawning an agent. diff --git a/src/triggers/github/review-requested.ts b/src/triggers/github/review-requested.ts index 8c60c4306..4215932c8 100644 --- a/src/triggers/github/review-requested.ts +++ b/src/triggers/github/review-requested.ts @@ -17,10 +17,33 @@ import { resolveWorkItemIdWithFallback } from './utils.js'; * * This trigger: * 1. Fires on `pull_request.review_requested` events - * 2. Rejects requests sent by CASCADE personas (loop prevention) + * 2. Rejects requests sent by CASCADE personas (loop prevention) — EXCEPT a + * self-directed request where `sender === requested_reviewer` (see below) * 3. Checks if the requested reviewer is a CASCADE persona (implementer OR reviewer) * 4. Fires the `review` agent with PR number and work item ID from DB lookup * + * ## Self-directed exemption (shared-reviewer-token contributor) + * + * When the sender IS the requested reviewer and both resolve to a CASCADE + * persona, the event is a human — whose GitHub account also holds the shared + * `GITHUB_TOKEN_REVIEWER` — re-requesting *their own* review. The sender + * loop-prevention guard exempts this case so it falls through to the normal + * dispatch path instead of being silently skipped. + * + * This exemption cannot reintroduce a bot loop, because: + * - **CASCADE never programmatically calls GitHub's "request reviewers" API.** + * There is no `requestReviewers` call anywhere in `src/`, so every + * `review_requested` event is human-initiated by construction. + * - **A review *submission* by a persona emits `pull_request_review`, not + * `review_requested`.** An agent finishing a review therefore cannot re-fire + * this trigger. + * - **Dispatch is deduped per PR+SHA** via `claimReviewDispatch` / + * `releaseReviewDispatch`, so even a duplicate delivery is a no-op. + * + * Cross-persona requests (`sender !== requested_reviewer`, e.g. an + * implementer-authored PR auto-assigning the reviewer persona) are NOT exempt + * and still skip with the loop-prevention reason. + * * Default: **disabled** (opt-in via trigger config). * * Registration: this may race with CheckSuiteSuccessTrigger for the same PR head SHA. @@ -58,13 +81,30 @@ export class ReviewRequestedTrigger implements TriggerHandler { if (!personasResult.ok) return personasResult.skip; const personas = personasResult.value; - // Skip review requests FROM CASCADE personas (self-loop prevention) + // Hoisted above the sender guard so a self-directed request can be + // detected before the loop-prevention skip decision is made. const senderLogin = payload.sender.login; - if (isCascadeBot(senderLogin, personas)) { + const requestedReviewer = payload.requested_reviewer?.login; + + // A self-directed request is one where the sender IS the requested + // reviewer. GitHub forbids requesting a review from the PR author, so the + // precise mechanic is `sender === requested_reviewer` (not authorship). + // This is always human-initiated: CASCADE never programmatically calls + // GitHub's "request reviewers" API (no `requestReviewers` in `src/`), and + // a persona *submitting* a review emits `pull_request_review`, not + // `review_requested` — so this cannot be a bot re-firing the trigger. + // Dispatch is additionally deduped per PR+SHA below, so a duplicate + // delivery is a no-op. See the class JSDoc for the full rationale. + const isSelfDirectedReviewRequest = !!requestedReviewer && senderLogin === requestedReviewer; + + // Skip review requests FROM CASCADE personas (self-loop prevention), + // EXCEPT self-directed requests (a human using the shared reviewer token + // re-requesting their own review), which fall through to dispatch. + if (isCascadeBot(senderLogin, personas) && !isSelfDirectedReviewRequest) { logger.info('Skipping review request from CASCADE persona (loop prevention)', { prNumber, sender: senderLogin, - requestedReviewer: payload.requested_reviewer?.login, + requestedReviewer, }); return skip( this.name, @@ -72,8 +112,9 @@ export class ReviewRequestedTrigger implements TriggerHandler { ); } - // Check if the requested reviewer is a CASCADE persona - const requestedReviewer = payload.requested_reviewer?.login; + // Check if the requested reviewer is a CASCADE persona. A non-persona + // self-request (sender === requested_reviewer but not a CASCADE persona) + // still skips here with the not-a-cascade-persona reason. if (!requestedReviewer) { logger.debug('No requested reviewer in payload, skipping', { prNumber }); return skip( diff --git a/tests/unit/triggers/review-requested.test.ts b/tests/unit/triggers/review-requested.test.ts index 49e96c335..5e3d66a68 100644 --- a/tests/unit/triggers/review-requested.test.ts +++ b/tests/unit/triggers/review-requested.test.ts @@ -124,7 +124,10 @@ describe('ReviewRequestedTrigger', () => { expectSkip(result); }); - it('returns null when sender is the implementer persona (loop prevention)', async () => { + it('returns null when sender is the implementer persona (cross-persona loop prevention)', async () => { + // Cross-persona: sender (cascade-impl) !== requested_reviewer + // (cascade-reviewer). This is an implementer-authored PR auto-assigning + // the reviewer — a bot loop that must still skip. const ctx: TriggerContext = { project: mockProject, source: 'github', @@ -138,7 +141,9 @@ describe('ReviewRequestedTrigger', () => { expectSkip(result); }); - it('returns null when sender is the reviewer persona (loop prevention)', async () => { + it('returns null when sender is the reviewer persona (cross-persona loop prevention)', async () => { + // Cross-persona: sender (cascade-reviewer) !== requested_reviewer + // (cascade-impl). Still a bot loop, so it must skip. const ctx: TriggerContext = { project: mockProject, source: 'github', @@ -152,7 +157,12 @@ describe('ReviewRequestedTrigger', () => { expectSkip(result); }); - it('returns null when a persona requests review from itself (loop prevention)', async () => { + it('dispatches review when a persona requests review from itself (self-directed, shared-reviewer-token contributor)', async () => { + // A human whose GitHub account also holds the shared GITHUB_TOKEN_REVIEWER + // re-requests their OWN review: sender === requested_reviewer, both a + // CASCADE persona. This is always human-initiated (CASCADE never + // programmatically requests reviewers), so it must fall through to + // dispatch instead of being skipped by the sender loop-prevention guard. const ctx: TriggerContext = { project: mockProject, source: 'github', @@ -163,7 +173,47 @@ describe('ReviewRequestedTrigger', () => { personaIdentities: mockPersonaIdentities, }; const result = await trigger.handle(ctx); - expectSkip(result); + expect(result).not.toBeNull(); + expect(result?.agentType).toBe('review'); + }); + + it('dispatches review for a reviewer self-request and runs the release-before-claim dedup sequence', async () => { + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: { + ...makeReviewRequestedPayload('cascade-reviewer'), + sender: { login: 'cascade-reviewer' }, + }, + personaIdentities: mockPersonaIdentities, + }; + const result = await trigger.handle(ctx); + expect(result?.agentType).toBe('review'); + // Human-initiated self-request supersedes any prior automated dispatch + // claim: release runs before claim, and the freed slot is re-claimed. + expect(mockReleaseReviewDispatch).toHaveBeenCalledWith('owner/repo:42:abc123'); + expect(mockClaimReviewDispatch).toHaveBeenCalledWith( + 'owner/repo:42:abc123', + 'review-requested', + expect.objectContaining({ prNumber: 42, headSha: 'abc123' }), + ); + }); + + it('still skips a non-persona self-request with the not-a-cascade-persona reason', async () => { + // sender === requested_reviewer, but 'human-x' is NOT a CASCADE persona. + // The self-directed exemption only bypasses the SENDER loop guard; the + // requested-reviewer persona gate still rejects a non-persona reviewer. + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: { + ...makeReviewRequestedPayload('human-x'), + sender: { login: 'human-x' }, + }, + personaIdentities: mockPersonaIdentities, + }; + const result = await trigger.handle(ctx); + expectSkip(result, /not a cascade persona/); }); it('returns null when requested reviewer is not a CASCADE persona', async () => { From 30b379acf2734e7735668e97024b2f51134dbb27 Mon Sep 17 00:00:00 2001 From: aaight Date: Fri, 10 Jul 2026 17:12:13 +0200 Subject: [PATCH 3/4] feat(jira): add optional authType config/credential field (#1481) Co-authored-by: Cascade Bot --- src/integrations/pm/jira/config-schema.ts | 15 +++ src/integrations/pm/jira/manifest.ts | 1 + src/jira/types.ts | 7 ++ src/pm/config.ts | 6 + .../pm/jira/manifest-config-schema.test.ts | 107 +++++++++++++++++- 5 files changed, 134 insertions(+), 2 deletions(-) diff --git a/src/integrations/pm/jira/config-schema.ts b/src/integrations/pm/jira/config-schema.ts index 174a90b50..c4e3cfc6d 100644 --- a/src/integrations/pm/jira/config-schema.ts +++ b/src/integrations/pm/jira/config-schema.ts @@ -26,6 +26,21 @@ export const jiraConfigSchema = z /** JIRA cloud base URL (e.g. "https://acme.atlassian.net"). */ baseUrl: z.string().url(), + /** + * Optional JIRA authentication mode. Non-secret connection setting + * (mirrors `baseUrl`), NOT a credential role. + * + * - `'basic'` — classic site-token mode (the historical default). + * - `'scoped'` — scoped gateway-token mode (API tokens with scopes). + * + * Both modes still authenticate via HTTP Basic with `email:api_token` + * (confirmed live in MNG-1735) — the enum distinguishes the token + * class, not a Basic-vs-Bearer scheme. Absent ⇒ treated as `'basic'`, + * so every existing saved config stays valid untouched. Later stories + * consume this field for host routing / client behavior. + */ + authType: z.enum(['basic', 'scoped']).optional(), + /** * Mapping from CASCADE status keys (backlog/todo/inProgress/done/...) * to JIRA status names or transition IDs. diff --git a/src/integrations/pm/jira/manifest.ts b/src/integrations/pm/jira/manifest.ts index 3b479b3c4..d1249511b 100644 --- a/src/integrations/pm/jira/manifest.ts +++ b/src/integrations/pm/jira/manifest.ts @@ -153,6 +153,7 @@ export const jiraManifest: PMProviderManifest = { configFixture: { projectKey: 'CASCADE', baseUrl: 'https://example.atlassian.net', + authType: 'basic', statuses: { backlog: 'Backlog', todo: 'To Do', done: 'Done' }, issueTypes: { task: 'Task' }, customFields: { cost: 'customfield_10100' }, diff --git a/src/jira/types.ts b/src/jira/types.ts index 1595db801..d055e0522 100644 --- a/src/jira/types.ts +++ b/src/jira/types.ts @@ -2,4 +2,11 @@ export interface JiraCredentials { email: string; apiToken: string; baseUrl: string; + /** + * Optional JIRA authentication mode carried through `withJiraCredentials`. + * Non-secret config (mirrors `baseUrl`), NOT a separate credential role. + * `'basic'` = classic site-token mode; `'scoped'` = scoped gateway-token + * mode. Absent ⇒ treated as `'basic'`. Later stories consume this field. + */ + authType?: 'basic' | 'scoped'; } diff --git a/src/pm/config.ts b/src/pm/config.ts index 3810ef398..60b0739ef 100644 --- a/src/pm/config.ts +++ b/src/pm/config.ts @@ -20,6 +20,12 @@ export interface TrelloConfig { export interface JiraConfig { projectKey: string; baseUrl: string; + /** + * Optional JIRA authentication mode (non-secret config, mirrors `baseUrl`). + * `'basic'` = classic site-token mode; `'scoped'` = scoped gateway-token + * mode. Absent ⇒ treated as `'basic'`. Later stories consume this field. + */ + authType?: 'basic' | 'scoped'; statuses: Record; issueTypes?: Record; customFields?: { cost?: string }; diff --git a/tests/unit/pm/jira/manifest-config-schema.test.ts b/tests/unit/pm/jira/manifest-config-schema.test.ts index 8d5965be8..61416ae2b 100644 --- a/tests/unit/pm/jira/manifest-config-schema.test.ts +++ b/tests/unit/pm/jira/manifest-config-schema.test.ts @@ -15,9 +15,15 @@ * covers project-scoped settings. */ -import { describe, expect, it } from 'vitest'; -import { jiraConfigSchema } from '../../../../src/integrations/pm/jira/config-schema.js'; +import { beforeAll, describe, expect, expectTypeOf, it } from 'vitest'; +import { registerBuiltInEngines } from '../../../../src/backends/bootstrap.js'; +import { ProjectConfigSchema } from '../../../../src/config/schema.js'; +import { + type JiraIntegrationConfig, + jiraConfigSchema, +} from '../../../../src/integrations/pm/jira/config-schema.js'; import { jiraManifest } from '../../../../src/integrations/pm/jira/manifest.js'; +import type { ProjectConfig } from '../../../../src/types/index.js'; const fullFixture = { projectKey: 'CASCADE', @@ -88,4 +94,101 @@ describe('jiraManifest exposes configSchema', () => { if (!schema) return; expect(() => schema.parse(jiraManifest.configFixture)).not.toThrow(); }); + + it("jiraManifest.configFixture includes authType: 'basic'", () => { + const schema = jiraManifest.configSchema; + expect(schema).toBeDefined(); + if (!schema) return; + const parsed = schema.parse(jiraManifest.configFixture) as JiraIntegrationConfig; + expect(parsed.authType).toBe('basic'); + }); +}); + +describe('jiraConfigSchema — optional authType (MNG-1736)', () => { + it("accepts authType: 'basic' and preserves it across a round-trip", () => { + const parsed1 = jiraConfigSchema.parse({ ...fullFixture, authType: 'basic' }); + expect(parsed1.authType).toBe('basic'); + const parsed2 = jiraConfigSchema.parse(JSON.parse(JSON.stringify(parsed1))); + expect(parsed2).toEqual(parsed1); + }); + + it("accepts authType: 'scoped' and preserves it across a round-trip", () => { + const parsed1 = jiraConfigSchema.parse({ ...fullFixture, authType: 'scoped' }); + expect(parsed1.authType).toBe('scoped'); + const parsed2 = jiraConfigSchema.parse(JSON.parse(JSON.stringify(parsed1))); + expect(parsed2).toEqual(parsed1); + }); + + it('treats absent authType as valid — existing configs without authType stay valid', () => { + // fullFixture carries no authType: this is the pre-MNG-1736 shape. + const parsed = jiraConfigSchema.parse(fullFixture); + expect(parsed.authType).toBeUndefined(); + // Optional-without-default: absent stays absent (no key injected), so the + // round-trip identity the conformance harness relies on stays green. + const reparsed = jiraConfigSchema.parse(JSON.parse(JSON.stringify(parsed))); + expect(reparsed).toEqual(parsed); + expect('authType' in reparsed).toBe(false); + }); + + it('rejects unknown authType values (no bearer/oauth enum expansion — MNG-1735)', () => { + expect(() => jiraConfigSchema.parse({ ...fullFixture, authType: 'bearer' })).toThrow(); + expect(() => jiraConfigSchema.parse({ ...fullFixture, authType: 'oauth' })).toThrow(); + expect(() => jiraConfigSchema.parse({ ...fullFixture, authType: '' })).toThrow(); + }); + + it('infers authType as an optional basic|scoped union on JiraIntegrationConfig', () => { + expectTypeOf().toEqualTypeOf< + 'basic' | 'scoped' | undefined + >(); + }); +}); + +describe('ProjectConfig-level authType flow (MNG-1736 — src/types/index.ts)', () => { + beforeAll(() => { + // ProjectConfigSchema references engine defaults elsewhere in the object; + // register the built-in engines so an unrelated default never interferes. + registerBuiltInEngines(); + }); + + it("central ProjectConfigSchema accepts a jira block carrying authType: 'scoped'", () => { + const result = ProjectConfigSchema.parse({ + id: 'p1', + orgId: 'org1', + name: 'Proj', + repo: 'owner/repo', + pm: { type: 'jira' }, + jira: { + projectKey: 'CASCADE', + baseUrl: 'https://acme.atlassian.net', + authType: 'scoped', + statuses: {}, + }, + }); + expect(result.jira?.authType).toBe('scoped'); + }); + + it('central ProjectConfigSchema still accepts a jira block without authType (backward compat)', () => { + const result = ProjectConfigSchema.parse({ + id: 'p2', + orgId: 'org1', + name: 'Proj', + repo: 'owner/repo', + pm: { type: 'jira' }, + jira: { + projectKey: 'CASCADE', + baseUrl: 'https://acme.atlassian.net', + statuses: {}, + }, + }); + expect(result.jira?.authType).toBeUndefined(); + }); + + it('ProjectConfig["jira"] type (src/types/index.ts) exposes optional authType', () => { + // ProjectConfig is z.infer, whose `jira` field + // derives from jiraConfigSchema — the single source of truth. Adding the + // field to the schema flows it here with no hand-written interface to edit. + expectTypeOf['authType']>().toEqualTypeOf< + 'basic' | 'scoped' | undefined + >(); + }); }); From b479e4abf4ce0bc90265f458138b7c6098ba6bca Mon Sep 17 00:00:00 2001 From: aaight Date: Fri, 10 Jul 2026 17:24:37 +0200 Subject: [PATCH 4/4] test(github-personas): integration coverage for self-directed review requests (#1482) Co-authored-by: Cascade Bot --- tests/integration/github-personas.test.ts | 59 ++++++++++++++++++++++- 1 file changed, 57 insertions(+), 2 deletions(-) diff --git a/tests/integration/github-personas.test.ts b/tests/integration/github-personas.test.ts index a20a2518a..5ab2f52b9 100644 --- a/tests/integration/github-personas.test.ts +++ b/tests/integration/github-personas.test.ts @@ -68,7 +68,19 @@ function makePRReviewPayload(overrides: { }; } -function makeReviewRequestedPayload(requestedReviewer: string, prAuthor: string) { +/** + * Builds a `review_requested` webhook payload. + * + * `senderLogin` defaults to `prAuthor` (the common case: someone requests a + * review of another user's PR). Pass an explicit `senderLogin` to model a + * self-directed request where `sender === requested_reviewer` — the + * shared-`GITHUB_TOKEN_REVIEWER` contributor re-requesting their own review. + */ +function makeReviewRequestedPayload( + requestedReviewer: string, + prAuthor: string, + senderLogin: string = prAuthor, +) { return { action: 'review_requested', number: 42, @@ -86,7 +98,7 @@ function makeReviewRequestedPayload(requestedReviewer: string, prAuthor: string) }, requested_reviewer: { login: requestedReviewer }, repository: { full_name: 'owner/repo', html_url: 'https://github.com/owner/repo' }, - sender: { login: prAuthor }, + sender: { login: senderLogin }, }; } @@ -398,6 +410,49 @@ describe('GitHub Dual-Persona System (integration)', () => { expect(result?.agentType).toBe('review'); }); + it('triggers review for a self-directed request (sender === requested_reviewer, shared reviewer token)', async () => { + await seedIntegration({ + category: 'scm', + provider: 'github', + config: {}, + }); + // Agent must be explicitly enabled for the trigger to fire + await seedAgentConfig({ agentType: 'review' }); + await seedTriggerConfig({ + agentType: 'review', + triggerEvent: 'scm:review-requested', + enabled: true, + }); + + const project = await findProjectByRepoFromDb('owner/repo'); + expect(project).toBeDefined(); + const trigger = new ReviewRequestedTrigger(); + + // Self-directed request: the reviewer persona is BOTH the sender and the + // requested reviewer (sender === requested_reviewer === TEST_PERSONAS.reviewer). + // This models a human contributor whose GitHub account also holds the shared + // GITHUB_TOKEN_REVIEWER re-requesting their own review. The self-directed + // exemption must let this fall through to dispatch instead of the + // loop-prevention skip that applies to cross-persona requests from a persona. + const payload = makeReviewRequestedPayload( + TEST_PERSONAS.reviewer, // requested_reviewer + 'external-user', // prAuthor (GitHub forbids requesting review from the PR author) + TEST_PERSONAS.reviewer, // sender === requested_reviewer + ); + const ctx: TriggerContext = { + project: assertFound(project), + source: 'github', + payload, + personaIdentities: TEST_PERSONAS, + }; + + // The sender IS a CASCADE persona, so a naive loop-prevention guard would + // skip; the self-directed exemption instead dispatches the review agent. + expect(trigger.matches(ctx)).toBe(true); + const result = await trigger.handle(ctx); + expect(result?.agentType).toBe('review'); + }); + it('returns null when non-persona reviewer is requested', async () => { await seedIntegration({ category: 'scm',