Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ cascade projects credentials-set <id> --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

Expand Down
2 changes: 2 additions & 0 deletions docs/architecture/03-trigger-system.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <handler> skipped: <message>`. 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.
Expand Down
19 changes: 18 additions & 1 deletion src/backends/secretOrchestrator.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { writeFileSync } from 'node:fs';
import {
createIntegrationChecker,
resolveEffectiveCapabilities,
Expand All @@ -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';
Expand Down Expand Up @@ -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');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional hardening: the file is written only for comment-only and never removed. Safe under one-job-per-ephemeral-container, but if /tmp is ever reused across runs (non-container execution or a future change), an all run would read a stale comment-only file and incorrectly downgrade the review. Writing the resolved policy unconditionally (both all and comment-only) makes the file self-correcting and removes the reliance on container ephemerality — the CLI's env>file>default precedence still holds.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — the policy file is now written unconditionally with the run's resolved policy (both all and comment-only) instead of only for comment-only.

It's self-correcting: an all run overwrites any stale comment-only file rather than inheriting it, removing the reliance on /tmp never being reused. The CLI's env > file > default precedence in resolveEventPolicyFromEnv() is unchanged.

Added tests in secretOrchestrator.test.ts asserting the write for the default (all), comment-only, and non-review-agent-on-a-comment-only-project (self-correcting) cases — mocking only writeFileSync so no real /tmp I/O happens during the run.

} 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,
Expand Down
32 changes: 28 additions & 4 deletions src/cli/scm/create-pr-review.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -31,10 +34,31 @@ async function deleteAckComment(owner: string, repo: string): Promise<boolean> {
}
}

/** 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) => {
Expand Down
10 changes: 10 additions & 0 deletions src/config/reviewEventPolicy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
15 changes: 15 additions & 0 deletions src/integrations/pm/jira/config-schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions src/integrations/pm/jira/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' },
Expand Down
7 changes: 7 additions & 0 deletions src/jira/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
}
6 changes: 6 additions & 0 deletions src/pm/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authType is added here (and to jiraConfigSchema, JiraCredentials, and the manifest fixture), but the runtime config-mapper drops it. buildJiraConfig in src/db/repositories/configMapper.ts:263 hand-picks fields and doesn't copy authType (its local JiraIntegrationConfig interface at line 24 and ProjectConfigRaw['jira'] at line 139 also omit it), and mapProjectRow returns ProjectConfigRaw directly (line 421) without re-parsing through ProjectConfigSchema. So ProjectConfig.jira.authType loaded from the DB is always undefined, regardless of what's persisted.

The new ProjectConfig-level authType flow test only exercises ProjectConfigSchema.parse(...), which isn't the runtime path — it passes green while the mapper silently omits the field. Since this is forward-looking ("later stories consume this"), nothing breaks today, but the later consumer will read undefined. Suggest adding authType to configMapper's JiraIntegrationConfig, ProjectConfigRaw['jira'], and buildJiraConfig now, or documenting the deferral so it isn't a surprise. Also note the src/jira/types.ts comment says it's "carried through withJiraCredentials" — no call site populates it yet, which is fine but slightly overstates current behavior.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — threaded authType through the runtime config-mapper so it survives the DB-load path.

src/db/repositories/configMapper.ts now carries it in the local JiraIntegrationConfig interface, in ProjectConfigRaw['jira'], and — critically — copies it in buildJiraConfig, which previously hand-picked fields and dropped authType before jiraConfigSchema re-parsed via validateConfig. So ProjectConfig.jira.authType now round-trips from the DB instead of always loading as undefined.

Added two mapper-level regression tests in configMapper.test.ts (preserves jira authType through the mapper / leaves jira authType undefined when not configured) that exercise the real mapProjectRow path, not just ProjectConfigSchema.parse.

Also reworded the src/jira/types.ts authType JSDoc so it no longer overstates: the field is reserved on JiraCredentials but no call site populates it yet (JiraIntegration.withCredentials still builds { email, apiToken, baseUrl } only).

statuses: Record<string, string>;
issueTypes?: Record<string, string>;
customFields?: { cost?: string };
Expand Down
53 changes: 47 additions & 6 deletions src/triggers/github/review-requested.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -58,22 +81,40 @@ 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,
`Review request on PR #${prNumber} sent BY cascade persona ${senderLogin} (loop prevention)`,
);
}

// 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(
Expand Down
59 changes: 57 additions & 2 deletions tests/integration/github-personas.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 },
};
}

Expand Down Expand Up @@ -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',
Expand Down
Loading
Loading