From dc93cb8ec91206c7a8097638a7e559bf58a08aee Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 14 Sep 2026 17:55:39 +0000 Subject: [PATCH 01/11] fix(ladon): enforce findings without automated approval --- ladon/arbiter/action.yml | 4 + ladon/arbiter/dist/index.js | 81 ++++- ladon/arbiter/src/enforce.test.ts | 81 +++++ ladon/arbiter/src/enforce.ts | 80 +++++ ladon/arbiter/src/index.ts | 28 ++ .../src/no-approval.integration.test.ts | 283 ++++++++++++++++++ ladon/arbiter/src/post.test.ts | 32 ++ ladon/arbiter/src/post.ts | 9 +- ladon/setup/action.yml | 6 + ladon/setup/dist/index.js | 22 +- ladon/setup/src/index.ts | 24 +- .../setup/src/no-approval.integration.test.ts | 210 +++++++++++++ ladon/setup/src/short-circuit.test.ts | 29 ++ ladon/setup/src/short-circuit.ts | 3 +- 14 files changed, 884 insertions(+), 8 deletions(-) create mode 100644 ladon/arbiter/src/no-approval.integration.test.ts create mode 100644 ladon/setup/src/no-approval.integration.test.ts diff --git a/ladon/arbiter/action.yml b/ladon/arbiter/action.yml index fdb785b..7c6413e 100644 --- a/ladon/arbiter/action.yml +++ b/ladon/arbiter/action.yml @@ -3,6 +3,10 @@ description: 'Decides approve/request-changes/comment/escalate from the reviewer author: 'Ad Context Protocol' inputs: + auto-approve: + description: 'Whether Ladon may submit APPROVE (true or false). Set false for findings without approving reviews; blocking findings and escalations fail the action.' + required: false + default: 'true' anthropic-api-key: description: 'Anthropic API key' required: true diff --git a/ladon/arbiter/dist/index.js b/ladon/arbiter/dist/index.js index b85ecb4..703aae5 100644 --- a/ladon/arbiter/dist/index.js +++ b/ladon/arbiter/dist/index.js @@ -47492,6 +47492,50 @@ const GATED_PATH_OVERRIDE_PREFIX = '> **[Override: forced from `approve` to `esc function enforceDecisionGuards(decision, ctx) { const overrides = []; let result = decision; + // In findings-only mode the action result is a policy check, so enforce the + // blocking rows of arbiter-decision.md before suppressing approval. An LLM + // "approve" or "comment" must not turn a known blocker into a green check. + if (ctx.autoApprove === false) { + const findings = ctx.findings ?? []; + const blockers = findings.filter((f) => f.severity === 'critical' || f.severity === 'high'); + const mediums = findings.filter((f) => f.severity === 'medium'); + const reasons = []; + if (ctx.highRisk && + ctx.highRiskReasons?.some((r) => r.includes('(deleted)'))) { + reasons.push('Deletion in a high-risk path requires human review.'); + } + if (mediums.some((f) => ['data-loss', 'schema', 'infra'].includes(f.category))) { + reasons.push('Medium finding in a sensitive category requires human review.'); + } + if (ctx.highRisk && + ctx.highRiskReasons?.some((r) => r.includes('(modified)')) && + mediums.length > 0) { + reasons.push('Medium finding with a modified high-risk path requires human review.'); + } + if (ctx.priorDecision?.outcome === 'escalate' && + findings.some((f) => f.severity !== 'low')) { + reasons.push('Prior escalation still has actionable findings.'); + } + if (ctx.gatedPaths && ctx.reviewDecision !== 'APPROVED') { + reasons.push('Gated paths require human/CODEOWNERS approval.'); + } + if (blockers.length > 0) { + result = { + ...result, + outcome: 'request-changes', + blocking_findings: blockers.map((f) => `${f.file}${f.line ? `:${f.line}` : ''} — ${f.title}`), + }; + overrides.push('Critical/high findings require changes regardless of the model verdict.'); + } + else if (reasons.length > 0 && result.outcome !== 'request-changes') { + result = { + ...result, + outcome: 'escalate', + escalation_reasons: [...result.escalation_reasons, ...reasons], + }; + overrides.push(...reasons); + } + } // Gated-paths mirrors the decision table's row 2 (see arbiter-decision.md): // the LLM is expected to apply that row directly and choose `escalate` // itself, exactly like the author-team HARD RULE below. This block is the @@ -47540,6 +47584,15 @@ function enforceDecisionGuards(decision, ctx) { summary: `${NO_AUTO_APPROVE_OVERRIDE_PREFIX}\n>\n> ${reason}\n\n${result.summary}`, }; } + if (ctx.autoApprove === false && result.outcome === 'approve') { + const reason = 'Automatic approval is disabled (auto-approve=false). Ladon findings do not satisfy the required human review.'; + overrides.push(reason); + result = { + ...result, + outcome: 'comment', + summary: `${reason}\n\n${result.summary}`, + }; + } return { decision: result, overrides }; } @@ -47658,7 +47711,12 @@ async function addLabel(params) { }); } async function postReview(params) { - const { octokit, owner, repo, prNumber, headSha, event, body } = params; + const { octokit, owner, repo, prNumber, headSha, body } = params; + // Last boundary before the API: never approve and then dismiss. Auto-merge + // can act on the first write, before a cleanup or a failing check completes. + const event = params.autoApprove === false && params.event === 'APPROVE' + ? 'COMMENT' + : params.event; try { await octokit.rest.pulls.createReview({ owner, @@ -47983,6 +48041,13 @@ function changeRequestIds(input) { } } async function main() { + // The manifest supplies the compatible default. Empty/invalid explicit input + // must fail before any API write, rather than enabling approvals by fallback. + const autoApproveInput = getInput('auto-approve', { required: true }); + if (autoApproveInput !== 'true' && autoApproveInput !== 'false') { + throw new Error('auto-approve must be exactly "true" or "false"'); + } + const autoApprove = autoApproveInput === 'true'; const anthropicApiKey = getInput('anthropic-api-key', { required: true }); const githubToken = getInput('github-token', { required: true }); const findingsRaw = getInput('findings-json', { required: true }); @@ -48056,6 +48121,8 @@ async function main() { recheckedReviewIds: activeChangeRequestIds, }); if (cleanup.headChanged) { + if (!autoApprove) + setFailed('PR head changed before review cleanup; rerun Ladon for the current head.'); info(`Skipping stale arbiter run for ${headSha}; the PR head changed before review cleanup.`); return; } @@ -48107,6 +48174,11 @@ async function main() { }); const enforced = enforceDecisionGuards(llmDecision, { authorTeamMatches, + autoApprove, + findings: findings.findings, + highRisk, + highRiskReasons, + priorDecision, gatedPaths, gatedPathsReasons, reviewDecision, @@ -48146,6 +48218,7 @@ async function main() { prNumber, headSha, event: mapOutcomeToReviewEvent(decision.outcome), + autoApprove, body, }); if (decision.outcome === 'escalate') { @@ -48162,6 +48235,12 @@ async function main() { } } setOutput('outcome', decision.outcome); + if (!autoApprove && + (decision.outcome === 'request-changes' || + decision.outcome === 'escalate' || + reviewCleanupWarning)) { + setFailed(`Ladon requires resolution or human review: ${decision.outcome}${reviewCleanupWarning ? `; ${reviewCleanupWarning}` : ''}`); + } } main().catch((err) => { setFailed(err instanceof Error ? err.message : String(err)); diff --git a/ladon/arbiter/src/enforce.test.ts b/ladon/arbiter/src/enforce.test.ts index 76172c2..420d84a 100644 --- a/ladon/arbiter/src/enforce.test.ts +++ b/ladon/arbiter/src/enforce.test.ts @@ -246,3 +246,84 @@ describe('enforceDecisionGuards', () => { expect(result.overrides[0]).not.toContain('example-org/security') }) }) + +describe('no-approval decision policy', () => { + test('converts a clean approve into an attributable comment', () => { + const result = enforceDecisionGuards(baseDecision, { + authorTeamMatches: [], + autoApprove: false, + }) + expect(result.decision.outcome).toBe('comment') + expect(result.decision.summary).toContain('auto-approve=false') + expect(result.decision.summary).toContain('Clean review.') + }) + + test.each(['request-changes', 'escalate'] as const)( + 'preserves %s', + (outcome) => { + expect( + enforceDecisionGuards( + { ...baseDecision, outcome }, + { + authorTeamMatches: [], + autoApprove: false, + }, + ).decision.outcome, + ).toBe(outcome) + }, + ) +}) + +test.each([ + { + highRisk: true, + highRiskReasons: ['src/a.ts (modified)'], + expected: 'escalate', + }, + { + highRisk: true, + highRiskReasons: ['src/a.ts (added)'], + expected: 'comment', + }, + { + priorDecision: { + head: 'old', + outcome: 'escalate' as const, + high_risk: true, + reasons: [], + }, + expected: 'escalate', + }, +])( + 'no-approval mode preserves medium policy: %j', + ({ expected, ...context }) => { + const result = enforceDecisionGuards(baseDecision, { + ...context, + autoApprove: false, + authorTeamMatches: [], + findings: [ + { + severity: 'medium', + category: 'operability', + file: 'src/a.ts', + title: 'Missing timeout', + rationale: 'A request can hang', + posted_inline: true, + }, + ], + }) + expect(result.decision.outcome).toBe(expected) + }, +) + +test('high-risk flag alone does not force escalation in no-approval mode', () => { + expect( + enforceDecisionGuards(baseDecision, { + autoApprove: false, + authorTeamMatches: [], + highRisk: true, + highRiskReasons: ['src/a.ts (modified)'], + findings: [], + }).decision.outcome, + ).toBe('comment') +}) diff --git a/ladon/arbiter/src/enforce.ts b/ladon/arbiter/src/enforce.ts index c2b66b8..888d5ee 100644 --- a/ladon/arbiter/src/enforce.ts +++ b/ladon/arbiter/src/enforce.ts @@ -1,7 +1,14 @@ import type { ArbiterDecision } from './anthropic.js' +import type { Finding } from './findings.js' +import type { PriorDecision } from './prior-decision.js' export interface EnforcementContext { authorTeamMatches: string[] + autoApprove?: boolean + findings?: Finding[] + highRisk?: boolean + highRiskReasons?: string[] + priorDecision?: PriorDecision | null gatedPaths?: boolean gatedPathsReasons?: string[] reviewDecision?: string @@ -24,6 +31,68 @@ export function enforceDecisionGuards( const overrides: string[] = [] let result = decision + // In findings-only mode the action result is a policy check, so enforce the + // blocking rows of arbiter-decision.md before suppressing approval. An LLM + // "approve" or "comment" must not turn a known blocker into a green check. + if (ctx.autoApprove === false) { + const findings = ctx.findings ?? [] + const blockers = findings.filter( + (f) => f.severity === 'critical' || f.severity === 'high', + ) + const mediums = findings.filter((f) => f.severity === 'medium') + const reasons: string[] = [] + if ( + ctx.highRisk && + ctx.highRiskReasons?.some((r) => r.includes('(deleted)')) + ) { + reasons.push('Deletion in a high-risk path requires human review.') + } + if ( + mediums.some((f) => ['data-loss', 'schema', 'infra'].includes(f.category)) + ) { + reasons.push( + 'Medium finding in a sensitive category requires human review.', + ) + } + if ( + ctx.highRisk && + ctx.highRiskReasons?.some((r) => r.includes('(modified)')) && + mediums.length > 0 + ) { + reasons.push( + 'Medium finding with a modified high-risk path requires human review.', + ) + } + if ( + ctx.priorDecision?.outcome === 'escalate' && + findings.some((f) => f.severity !== 'low') + ) { + reasons.push('Prior escalation still has actionable findings.') + } + if (ctx.gatedPaths && ctx.reviewDecision !== 'APPROVED') { + reasons.push('Gated paths require human/CODEOWNERS approval.') + } + if (blockers.length > 0) { + result = { + ...result, + outcome: 'request-changes', + blocking_findings: blockers.map( + (f) => `${f.file}${f.line ? `:${f.line}` : ''} — ${f.title}`, + ), + } + overrides.push( + 'Critical/high findings require changes regardless of the model verdict.', + ) + } else if (reasons.length > 0 && result.outcome !== 'request-changes') { + result = { + ...result, + outcome: 'escalate', + escalation_reasons: [...result.escalation_reasons, ...reasons], + } + overrides.push(...reasons) + } + } + // Gated-paths mirrors the decision table's row 2 (see arbiter-decision.md): // the LLM is expected to apply that row directly and choose `escalate` // itself, exactly like the author-team HARD RULE below. This block is the @@ -77,5 +146,16 @@ export function enforceDecisionGuards( } } + if (ctx.autoApprove === false && result.outcome === 'approve') { + const reason = + 'Automatic approval is disabled (auto-approve=false). Ladon findings do not satisfy the required human review.' + overrides.push(reason) + result = { + ...result, + outcome: 'comment', + summary: `${reason}\n\n${result.summary}`, + } + } + return { decision: result, overrides } } diff --git a/ladon/arbiter/src/index.ts b/ladon/arbiter/src/index.ts index 3b54a60..0fa333a 100644 --- a/ladon/arbiter/src/index.ts +++ b/ladon/arbiter/src/index.ts @@ -47,6 +47,14 @@ function changeRequestIds(input: string): number[] { } async function main(): Promise { + // The manifest supplies the compatible default. Empty/invalid explicit input + // must fail before any API write, rather than enabling approvals by fallback. + const autoApproveInput = core.getInput('auto-approve', { required: true }) + if (autoApproveInput !== 'true' && autoApproveInput !== 'false') { + throw new Error('auto-approve must be exactly "true" or "false"') + } + const autoApprove = autoApproveInput === 'true' + const anthropicApiKey = core.getInput('anthropic-api-key', { required: true }) const githubToken = core.getInput('github-token', { required: true }) const findingsRaw = core.getInput('findings-json', { required: true }) @@ -133,6 +141,10 @@ async function main(): Promise { recheckedReviewIds: activeChangeRequestIds, }) if (cleanup.headChanged) { + if (!autoApprove) + core.setFailed( + 'PR head changed before review cleanup; rerun Ladon for the current head.', + ) core.info( `Skipping stale arbiter run for ${headSha}; the PR head changed before review cleanup.`, ) @@ -195,6 +207,11 @@ async function main(): Promise { const enforced = enforceDecisionGuards(llmDecision, { authorTeamMatches, + autoApprove, + findings: findings.findings, + highRisk, + highRiskReasons, + priorDecision, gatedPaths, gatedPathsReasons, reviewDecision, @@ -237,6 +254,7 @@ async function main(): Promise { prNumber, headSha, event: mapOutcomeToReviewEvent(decision.outcome), + autoApprove, body, }) @@ -255,6 +273,16 @@ async function main(): Promise { } core.setOutput('outcome', decision.outcome) + if ( + !autoApprove && + (decision.outcome === 'request-changes' || + decision.outcome === 'escalate' || + reviewCleanupWarning) + ) { + core.setFailed( + `Ladon requires resolution or human review: ${decision.outcome}${reviewCleanupWarning ? `; ${reviewCleanupWarning}` : ''}`, + ) + } } main().catch((err: unknown) => { diff --git a/ladon/arbiter/src/no-approval.integration.test.ts b/ladon/arbiter/src/no-approval.integration.test.ts new file mode 100644 index 0000000..9648f43 --- /dev/null +++ b/ladon/arbiter/src/no-approval.integration.test.ts @@ -0,0 +1,283 @@ +import { beforeEach, describe, expect, test, vi } from 'vitest' +import type { ArbiterDecision } from './anthropic.js' +import type { Finding } from './findings.js' + +const fixture = vi.hoisted(() => { + const inputs: Record = {} + const core = { + getInput: vi.fn((name: string) => inputs[name] ?? ''), + setOutput: vi.fn(), + setFailed: vi.fn(), + warning: vi.fn(), + info: vi.fn(), + } + const createReview = vi.fn() + const dismissReview = vi.fn() + const octokit = { + paginate: vi.fn(), + graphql: vi.fn(), + request: vi.fn(), + rest: { + pulls: { + createReview, + dismissReview, + listReviews: vi.fn(), + get: vi.fn(), + }, + issues: { getLabel: vi.fn(), addLabels: vi.fn() }, + teams: { getMembershipForUserInOrg: vi.fn() }, + }, + } + const context = { + repo: { owner: 'o', repo: 'r' }, + payload: { + pull_request: { user: { login: 'author' }, base: { ref: 'main' } }, + }, + } + return { inputs, core, octokit, context, decide: vi.fn() } +}) + +vi.mock('@actions/core', () => fixture.core) +vi.mock('@actions/github', () => ({ + context: fixture.context, + getOctokit: () => fixture.octokit, +})) +vi.mock('./anthropic.js', () => ({ decideViaAnthropic: fixture.decide })) +vi.mock('./diff-stats.js', () => ({ + computeDiffStatsFromFile: async () => ({ + fileCount: 1, + additions: 1, + deletions: 0, + files: ['src/auth.ts'], + }), +})) + +const clean: ArbiterDecision = { + outcome: 'approve', + summary: 'Clean review.', + blocking_findings: [], + escalation_reasons: [], +} +const finding = ( + severity: Finding['severity'], + category: Finding['category'] = 'security', +): Finding => ({ + severity, + category, + title: 'Authorization bypass', + rationale: 'Missing access check', + file: 'src/auth.ts', + line: 12, + posted_inline: true, +}) + +async function run() { + await import('./index.js') + await vi.waitFor(() => { + expect( + fixture.core.setFailed.mock.calls.length + + fixture.core.setOutput.mock.calls.length, + ).toBeGreaterThan(0) + }) +} + +beforeEach(() => { + vi.resetModules() + vi.clearAllMocks() + for (const key of Object.keys(fixture.inputs)) delete fixture.inputs[key] + Object.assign(fixture.inputs, { + 'auto-approve': 'false', + 'anthropic-api-key': 'test', + 'github-token': 'test', + 'findings-json': JSON.stringify({ + summary: 'Complete review', + findings: [], + }), + 'pr-number': '2911', + 'head-sha': 'exact-head', + 'base-sha': 'base', + 'diff-full-path': 'unused', + 'ladon-bot-login': 'aao-secretariat[bot]', + }) + fixture.context.payload.pull_request.user.login = 'author' + fixture.decide.mockResolvedValue(clean) + fixture.octokit.paginate.mockResolvedValue([]) + fixture.octokit.rest.pulls.get.mockResolvedValue({ + data: { head: { sha: 'exact-head' } }, + }) + fixture.octokit.rest.pulls.createReview.mockImplementation( + async (request) => { + // Observe the first API write, before status completion or any cleanup. + if (fixture.inputs['auto-approve'] === 'false') + expect(request.event).not.toBe('APPROVE') + return { data: { id: 1 } } + }, + ) + fixture.octokit.rest.issues.getLabel.mockResolvedValue({ data: {} }) + fixture.octokit.rest.issues.addLabels.mockResolvedValue({ data: {} }) +}) + +describe('arbiter API and action result integration', () => { + test('compatible true mode still approves a clean review', async () => { + fixture.inputs['auto-approve'] = 'true' + await run() + expect(fixture.octokit.rest.pulls.createReview).toHaveBeenCalledWith( + expect.objectContaining({ event: 'APPROVE', commit_id: 'exact-head' }), + ) + expect(fixture.core.setFailed).not.toHaveBeenCalled() + }) + + test.each(['author', 'aao-secretariat[bot]', 'other[bot]'])( + 'clean no-approval review for %s is COMMENT, even for its own PR', + async (author) => { + fixture.context.payload.pull_request.user.login = author + await run() + expect( + fixture.octokit.rest.pulls.createReview, + ).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ + event: 'COMMENT', + commit_id: 'exact-head', + body: expect.stringContaining('auto-approve=false'), + }), + ) + const body = fixture.octokit.rest.pulls.createReview.mock.calls[0][0].body + expect(body).toContain('Ladon verdict: Comment') + expect(body).toContain('"outcome":"comment"') + expect(fixture.core.setOutput).toHaveBeenCalledWith('outcome', 'comment') + expect(fixture.core.setFailed).not.toHaveBeenCalled() + expect(fixture.octokit.rest.pulls.dismissReview).not.toHaveBeenCalled() + }, + ) + + test.each(['critical', 'high'] as const)( + 'a %s finding requests changes and fails even when the model approves', + async (severity) => { + fixture.inputs['findings-json'] = JSON.stringify({ + summary: 'Blocking finding', + findings: [finding(severity)], + }) + await run() + expect(fixture.octokit.rest.pulls.createReview).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'REQUEST_CHANGES', + body: expect.stringContaining( + 'src/auth.ts:12 — Authorization bypass', + ), + }), + ) + expect(fixture.core.setFailed).toHaveBeenCalledWith( + expect.stringContaining('request-changes'), + ) + }, + ) + + test('nonblocking medium findings remain in the attributable COMMENT', async () => { + fixture.inputs['findings-json'] = JSON.stringify({ + summary: 'One finding', + findings: [finding('medium')], + }) + await run() + expect(fixture.octokit.rest.pulls.createReview).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'COMMENT', + body: expect.stringContaining('src/auth.ts:12 — Authorization bypass'), + }), + ) + expect(fixture.core.setFailed).not.toHaveBeenCalled() + }) + + test.each([ + { 'high-risk': 'true', 'high-risk-reasons': '["src/auth.ts (deleted)"]' }, + { 'gated-paths': 'true', 'review-decision': 'REVIEW_REQUIRED' }, + { 'gated-paths': 'true', 'review-decision': 'APPROVED' }, + { 'gated-paths': 'true', 'review-decision': '' }, + ])( + 'policy escalation fails and cannot become a clean comment: %j', + async (inputs) => { + Object.assign(fixture.inputs, inputs) + // Even an aggregate APPROVED from Ladon/other bots is not human sign-off. + fixture.octokit.paginate.mockResolvedValue([ + { state: 'APPROVED', user: { login: 'other[bot]', type: 'Bot' } }, + ]) + fixture.decide.mockResolvedValue({ ...clean, outcome: 'comment' }) + await run() + expect(fixture.octokit.rest.pulls.createReview).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'COMMENT', + body: expect.stringContaining('Escalate to human review'), + }), + ) + expect(fixture.core.setFailed).toHaveBeenCalledWith( + expect.stringContaining('escalate'), + ) + expect(fixture.octokit.rest.issues.addLabels).toHaveBeenCalled() + }, + ) + + test.each(['data-loss', 'schema', 'infra'] as const)( + 'sensitive medium %s findings still escalate', + async (category) => { + fixture.inputs['findings-json'] = JSON.stringify({ + summary: 'Finding', + findings: [finding('medium', category)], + }) + await run() + expect(fixture.core.setOutput).toHaveBeenCalledWith('outcome', 'escalate') + expect(fixture.core.setFailed).toHaveBeenCalled() + }, + ) + + test('a prior dismissed approval cannot restore approval in the arbiter', async () => { + fixture.inputs['prior-decision'] = JSON.stringify({ + head: 'old-head', + outcome: 'approve', + high_risk: false, + reasons: [], + findings: [], + }) + await run() + expect( + fixture.octokit.rest.pulls.createReview, + ).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ event: 'COMMENT' }), + ) + }) + + test('the author team gate still yields COMMENT', async () => { + fixture.inputs['no-auto-approve-teams'] = 'o/security' + fixture.octokit.rest.teams.getMembershipForUserInOrg.mockResolvedValue({ + data: { state: 'active' }, + }) + await run() + expect(fixture.octokit.rest.pulls.createReview).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'COMMENT', + body: expect.stringContaining('o/security'), + }), + ) + }) + + test.each(['', 'False', '0', 'disabled', 'unresolved-expression'])( + 'invalid auto-approve=%j fails before any review API call', + async (input) => { + fixture.inputs['auto-approve'] = input + await run() + expect(fixture.core.setFailed).toHaveBeenCalled() + expect(fixture.decide).not.toHaveBeenCalled() + expect(fixture.octokit.rest.pulls.createReview).not.toHaveBeenCalled() + }, + ) + + test('review API failure fails the check with no retry to APPROVE', async () => { + fixture.octokit.rest.pulls.createReview.mockRejectedValue( + Object.assign(new Error('rejected'), { status: 422 }), + ) + await run() + expect(fixture.octokit.rest.pulls.createReview).toHaveBeenCalledTimes(1) + expect(fixture.octokit.rest.pulls.createReview.mock.calls[0][0].event).toBe( + 'COMMENT', + ) + expect(fixture.core.setFailed).toHaveBeenCalledWith('rejected') + }) +}) diff --git a/ladon/arbiter/src/post.test.ts b/ladon/arbiter/src/post.test.ts index 4334d7c..7dc5f5e 100644 --- a/ladon/arbiter/src/post.test.ts +++ b/ladon/arbiter/src/post.test.ts @@ -309,3 +309,35 @@ describe('hasHumanApproval', () => { ).resolves.toBe(true) }) }) + +describe('no-approval posting boundary', () => { + test.each(['APPROVE', 'REQUEST_CHANGES', 'COMMENT'] as const)( + '%s never sends APPROVE when autoApprove is false', + async (event) => { + // Record each call as it happens: an APPROVE followed by cleanup is unsafe. + const events: string[] = [] + const createReview = vi.fn(async (request) => { + events.push(request.event) + expect(request.event).not.toBe('APPROVE') + return { data: { id: 1 } } + }) + await postReview({ + octokit: { rest: { pulls: { createReview } } } as never, + owner: 'o', + repo: 'r', + prNumber: 1, + headSha: 'exact-head', + event, + body: 'Ladon findings', + autoApprove: false, + }) + expect(events).toEqual([event === 'APPROVE' ? 'COMMENT' : event]) + expect(createReview).toHaveBeenCalledWith( + expect.objectContaining({ + commit_id: 'exact-head', + body: 'Ladon findings', + }), + ) + }, + ) +}) diff --git a/ladon/arbiter/src/post.ts b/ladon/arbiter/src/post.ts index 3de7e94..0734855 100644 --- a/ladon/arbiter/src/post.ts +++ b/ladon/arbiter/src/post.ts @@ -69,9 +69,16 @@ export async function postReview(params: { prNumber: number headSha: string event: ReviewEvent + autoApprove?: boolean body: string }): Promise { - const { octokit, owner, repo, prNumber, headSha, event, body } = params + const { octokit, owner, repo, prNumber, headSha, body } = params + // Last boundary before the API: never approve and then dismiss. Auto-merge + // can act on the first write, before a cleanup or a failing check completes. + const event = + params.autoApprove === false && params.event === 'APPROVE' + ? 'COMMENT' + : params.event try { await octokit.rest.pulls.createReview({ owner, diff --git a/ladon/setup/action.yml b/ladon/setup/action.yml index 52981e2..fc930e1 100644 --- a/ladon/setup/action.yml +++ b/ladon/setup/action.yml @@ -3,6 +3,10 @@ description: 'Parses LADON.md, computes diff/delta, evaluates short-circuit cond author: 'Ad Context Protocol' inputs: + auto-approve: + description: 'Whether Ladon may submit APPROVE (true or false). Set false for findings without approving reviews; blocking findings and escalations fail the action.' + required: false + default: 'true' github-token: description: 'GitHub App installation token' required: true @@ -48,6 +52,8 @@ inputs: default: 'ladon/force-review' outputs: + reapprove: + description: 'true only when auto-approve is enabled and a prior approval can be renewed after an empty delta or pure rebase' should-run: description: 'true if downstream reviewer + arbiter should run' skip-reason: diff --git a/ladon/setup/dist/index.js b/ladon/setup/dist/index.js index 5b995f8..5247aea 100644 --- a/ladon/setup/dist/index.js +++ b/ladon/setup/dist/index.js @@ -39816,7 +39816,7 @@ function evaluateShortCircuit(ctx) { return { shouldRun: false, skipReason: 'merge-conflicts' }; if (ctx.releaseStackBranches.includes(ctx.headRef)) return { shouldRun: false, skipReason: 'release-stack-branch' }; - if (ctx.eventAction === 'synchronize') { + if (ctx.eventAction === 'synchronize' && ctx.autoApprove !== false) { if (ctx.isPureRebase) return { shouldRun: false, skipReason: 'pure-rebase' }; if (ctx.deltaFiles.length === 0 && @@ -39943,6 +39943,13 @@ async function fetchReviewDecision(params) { } } async function main() { + // The manifest supplies the compatible default. Empty/invalid explicit input + // must fail before any API write, rather than enabling approvals by fallback. + const autoApproveInput = getInput('auto-approve', { required: true }); + if (autoApproveInput !== 'true' && autoApproveInput !== 'false') { + throw new Error('auto-approve must be exactly "true" or "false"'); + } + const autoApprove = autoApproveInput === 'true'; const githubToken = getInput('github-token', { required: true }); const inputs = { highRiskPaths: csv(getInput('high-risk-paths')), @@ -40078,6 +40085,7 @@ async function main() { trivialGlobs: config.trivialPaths, }); const decision = evaluateShortCircuit({ + autoApprove, prState, isDraft, hasForceReviewLabel, @@ -40113,8 +40121,9 @@ async function main() { marker: 'ladon/merge-conflict-noted', }); } - else if (decision.skipReason === 'empty-delta' || - decision.skipReason === 'pure-rebase') { + else if (autoApprove && + (decision.skipReason === 'empty-delta' || + decision.skipReason === 'pure-rebase')) { // When a no-delta push dismisses a prior approval via GitHub's stale-review // policy, the PR stays blocked forever: nothing new to review, but the // approval is gone. Detect this and signal the orchestrator to re-submit it. @@ -40152,6 +40161,13 @@ async function main() { setOutput('skip-reason', decision.skipReason ?? ''); return; } + // With approvals disabled, a no-delta push must recheck the full surface: + // a previous failure/escalation cannot become a green check just by rebasing + // or changing a trivial file. Never reuse a dismissed approval as a verdict. + if (!autoApprove && + (isPureRebase || deltaFilesAfterTrivialFilter.length === 0)) { + deltaFiles = surfacePaths; + } // High-risk / gated-path evaluation runs on the PR surface. Change kinds // come from the pulls.listFiles response already fetched above — no extra // API call and, crucially, no fetch of the PR head. diff --git a/ladon/setup/src/index.ts b/ladon/setup/src/index.ts index 22b9a46..530eff5 100644 --- a/ladon/setup/src/index.ts +++ b/ladon/setup/src/index.ts @@ -163,6 +163,14 @@ async function fetchReviewDecision(params: { } async function main(): Promise { + // The manifest supplies the compatible default. Empty/invalid explicit input + // must fail before any API write, rather than enabling approvals by fallback. + const autoApproveInput = core.getInput('auto-approve', { required: true }) + if (autoApproveInput !== 'true' && autoApproveInput !== 'false') { + throw new Error('auto-approve must be exactly "true" or "false"') + } + const autoApprove = autoApproveInput === 'true' + const githubToken = core.getInput('github-token', { required: true }) const inputs: ActionInputs = { highRiskPaths: csv(core.getInput('high-risk-paths')), @@ -321,6 +329,7 @@ async function main(): Promise { }) const decision = evaluateShortCircuit({ + autoApprove, prState, isDraft, hasForceReviewLabel, @@ -356,8 +365,9 @@ async function main(): Promise { marker: 'ladon/merge-conflict-noted', }) } else if ( - decision.skipReason === 'empty-delta' || - decision.skipReason === 'pure-rebase' + autoApprove && + (decision.skipReason === 'empty-delta' || + decision.skipReason === 'pure-rebase') ) { // When a no-delta push dismisses a prior approval via GitHub's stale-review // policy, the PR stays blocked forever: nothing new to review, but the @@ -399,6 +409,16 @@ async function main(): Promise { return } + // With approvals disabled, a no-delta push must recheck the full surface: + // a previous failure/escalation cannot become a green check just by rebasing + // or changing a trivial file. Never reuse a dismissed approval as a verdict. + if ( + !autoApprove && + (isPureRebase || deltaFilesAfterTrivialFilter.length === 0) + ) { + deltaFiles = surfacePaths + } + // High-risk / gated-path evaluation runs on the PR surface. Change kinds // come from the pulls.listFiles response already fetched above — no extra // API call and, crucially, no fetch of the PR head. diff --git a/ladon/setup/src/no-approval.integration.test.ts b/ladon/setup/src/no-approval.integration.test.ts new file mode 100644 index 0000000..56e1644 --- /dev/null +++ b/ladon/setup/src/no-approval.integration.test.ts @@ -0,0 +1,210 @@ +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterAll, beforeEach, describe, expect, test, vi } from 'vitest' + +const fixture = vi.hoisted(() => { + const inputs: Record = {} + const outputs: Record = {} + const core = { + getInput: vi.fn((name: string) => inputs[name] ?? ''), + setOutput: vi.fn((name: string, value: string) => { + outputs[name] = value + }), + setFailed: vi.fn(), + warning: vi.fn(), + info: vi.fn(), + } + const octokit = { + paginate: vi.fn(), + graphql: vi.fn(), + request: vi.fn(), + rest: { + pulls: { + listFiles: vi.fn(), + listReviews: vi.fn(), + get: vi.fn(), + createReview: vi.fn(), + }, + repos: { compareCommitsWithBasehead: vi.fn() }, + issues: { listComments: vi.fn(), createComment: vi.fn() }, + git: { getCommit: vi.fn() }, + }, + } + const context = { + repo: { owner: 'o', repo: 'r' }, + eventName: 'pull_request_target', + actor: 'author', + payload: { + action: 'synchronize', + pull_request: { + number: 2911, + head: { sha: 'exact-head', ref: 'fix' }, + base: { sha: 'base', ref: 'main' }, + draft: false, + state: 'open', + user: { login: 'author' }, + labels: [], + }, + }, + } + return { inputs, outputs, core, octokit, context } +}) +vi.mock('@actions/core', () => fixture.core) +vi.mock('@actions/github', () => ({ + context: fixture.context, + getOctokit: () => fixture.octokit, +})) + +const directory = mkdtempSync(join(tmpdir(), 'ladon-policy-test-')) +writeFileSync( + join(directory, 'LADON.md'), + '## High-Risk Paths\n- src/**\n\n## Gated Paths\n- src/**\n', +) +afterAll(() => { + vi.unstubAllEnvs() + rmSync(directory, { recursive: true, force: true }) +}) + +async function run() { + await import('./index.js') + await vi.waitFor(() => { + expect( + fixture.core.setFailed.mock.calls.length > 0 || + fixture.outputs['skip-reason'] !== undefined, + ).toBe(true) + }) +} + +beforeEach(() => { + vi.resetModules() + vi.clearAllMocks() + vi.stubEnv('GITHUB_WORKSPACE', directory) + for (const object of [fixture.inputs, fixture.outputs]) { + for (const key of Object.keys(object)) delete object[key] + } + Object.assign(fixture.inputs, { + 'auto-approve': 'false', + 'github-token': 'test', + 'ladon-bot-login': 'aao-secretariat[bot]', + }) + fixture.context.payload.action = 'synchronize' + fixture.context.payload.pull_request.user.login = 'author' + fixture.context.actor = 'author' + fixture.octokit.rest.pulls.get.mockResolvedValue({ + data: { mergeable: true }, + }) + fixture.octokit.rest.repos.compareCommitsWithBasehead.mockResolvedValue({ + data: { files: [], commits: [], total_commits: 0 }, + }) + fixture.octokit.graphql.mockResolvedValue({ + repository: { pullRequest: { reviewDecision: 'REVIEW_REQUIRED' } }, + }) + fixture.octokit.request.mockResolvedValue({ + data: 'diff --git a/src/auth.ts b/src/auth.ts\n', + }) + fixture.octokit.paginate.mockImplementation(async (method) => { + if (method === fixture.octokit.rest.pulls.listFiles) + return [{ filename: 'src/auth.ts', status: 'modified' }] + if (method === fixture.octokit.rest.pulls.listReviews) + return [ + { + id: 1, + user: { login: 'aao-secretariat[bot]' }, + state: 'DISMISSED', + commit_id: 'old-head', + body: '', + }, + ] + return [] + }) +}) + +describe('setup stale-approval integration', () => { + test('compatible true mode still signals reapproval after stale dismissal', async () => { + fixture.inputs['auto-approve'] = 'true' + await run() + expect(fixture.outputs).toMatchObject({ + reapprove: 'true', + 'should-run': 'false', + 'skip-reason': 'pure-rebase', + 'head-sha': 'exact-head', + }) + }) + + test.each(['pure-rebase', 'empty-delta', 'trivial-only', 'tree-rebase'])( + '%s cannot renew a dismissed approval or skip policy in disabled mode', + async (scenario) => { + if (scenario === 'empty-delta') + fixture.octokit.rest.repos.compareCommitsWithBasehead.mockResolvedValue( + { data: { files: [{ filename: 'unrelated.ts' }] } }, + ) + if (scenario === 'trivial-only') { + fixture.octokit.paginate.mockResolvedValueOnce([ + { filename: 'src/auth.ts', status: 'modified' }, + { filename: 'README.md', status: 'modified' }, + ]) + fixture.octokit.rest.repos.compareCommitsWithBasehead.mockResolvedValue( + { data: { files: [{ filename: 'README.md' }] } }, + ) + } + if (scenario === 'tree-rebase') { + fixture.octokit.rest.repos.compareCommitsWithBasehead.mockRejectedValue( + new Error('unreachable'), + ) + fixture.octokit.rest.git.getCommit.mockResolvedValue({ + data: { tree: { sha: 'same-tree' } }, + }) + } + await run() + expect(fixture.outputs).toMatchObject({ + 'should-run': 'true', + 'high-risk': 'true', + 'gated-paths': 'true', + 'head-sha': 'exact-head', + }) + expect(fixture.outputs.reapprove).toBeUndefined() + expect(fixture.outputs['diff-delta-path']).toBe( + fixture.outputs['diff-full-path'], + ) + expect(fixture.octokit.rest.pulls.createReview).not.toHaveBeenCalled() + expect(fixture.core.setFailed).not.toHaveBeenCalled() + }, + ) + + test('normal opened review runs with approval disabled', async () => { + fixture.context.payload.action = 'opened' + await run() + expect(fixture.outputs['should-run']).toBe('true') + expect(fixture.outputs.reapprove).toBeUndefined() + }) + + test('bot author skips without reapproval', async () => { + fixture.context.payload.pull_request.user.login = 'dependabot[bot]' + await run() + expect(fixture.outputs).toMatchObject({ + 'should-run': 'false', + 'skip-reason': 'bot-author', + }) + expect(fixture.outputs.reapprove).toBeUndefined() + expect(fixture.octokit.rest.pulls.createReview).not.toHaveBeenCalled() + }) + + test('self-trigger skips without reapproval', async () => { + fixture.context.actor = 'aao-secretariat[bot]' + await run() + expect(fixture.outputs['skip-reason']).toBe('self-triggered') + expect(fixture.outputs.reapprove).toBeUndefined() + }) + + test.each(['', 'FALSE', 'off'])( + 'invalid input %j fails closed', + async (input) => { + fixture.inputs['auto-approve'] = input + await run() + expect(fixture.core.setFailed).toHaveBeenCalled() + expect(fixture.outputs.reapprove).toBeUndefined() + expect(fixture.octokit.rest.pulls.createReview).not.toHaveBeenCalled() + }, + ) +}) diff --git a/ladon/setup/src/short-circuit.test.ts b/ladon/setup/src/short-circuit.test.ts index 532ed80..20f2465 100644 --- a/ladon/setup/src/short-circuit.test.ts +++ b/ladon/setup/src/short-circuit.test.ts @@ -163,3 +163,32 @@ describe('evaluateShortCircuit', () => { ).toEqual({ shouldRun: false, skipReason: 'pr-closed' }) }) }) + +describe('no-approval mode', () => { + test.each([ + { isPureRebase: true }, + { deltaFiles: [], deltaFilesAfterTrivialFilter: [] }, + { deltaFiles: ['README.md'], deltaFilesAfterTrivialFilter: [] }, + ])('re-evaluates policy after a no-delta push: %j', (delta) => { + expect( + evaluateShortCircuit( + ctx({ + ...delta, + eventAction: 'synchronize', + autoApprove: false, + }), + ), + ).toEqual({ shouldRun: true, skipReason: null }) + }) + + test('still skips a configured bot author without approving it', () => { + expect( + evaluateShortCircuit( + ctx({ + authorLogin: 'dependabot[bot]', + autoApprove: false, + }), + ), + ).toEqual({ shouldRun: false, skipReason: 'bot-author' }) + }) +}) diff --git a/ladon/setup/src/short-circuit.ts b/ladon/setup/src/short-circuit.ts index 0ff5324..439055b 100644 --- a/ladon/setup/src/short-circuit.ts +++ b/ladon/setup/src/short-circuit.ts @@ -9,6 +9,7 @@ export type SkipReason = | 'empty-delta' export interface ShortCircuitContext { + autoApprove?: boolean prState: 'open' | 'closed' isDraft: boolean hasForceReviewLabel: boolean @@ -45,7 +46,7 @@ export function evaluateShortCircuit( if (!ctx.mergeable) return { shouldRun: false, skipReason: 'merge-conflicts' } if (ctx.releaseStackBranches.includes(ctx.headRef)) return { shouldRun: false, skipReason: 'release-stack-branch' } - if (ctx.eventAction === 'synchronize') { + if (ctx.eventAction === 'synchronize' && ctx.autoApprove !== false) { if (ctx.isPureRebase) return { shouldRun: false, skipReason: 'pure-rebase' } if ( ctx.deltaFiles.length === 0 && From 1c8b1021f4c742e7f0188687447a73da87114d4a Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 14 Sep 2026 17:56:21 +0000 Subject: [PATCH 02/11] fix(ladon): pin and guard no-approval orchestration --- internal/versioning/review-action.test.ts | 160 +++++++++++++++++ ladon/HUMAN-APPROVAL-ADOPTION.md | 207 ++++++++++++++++++++++ ladon/INSTALL.md | 24 ++- ladon/README.md | 8 + ladon/review/action.yml | 39 +++- 5 files changed, 423 insertions(+), 15 deletions(-) create mode 100644 ladon/HUMAN-APPROVAL-ADOPTION.md diff --git a/internal/versioning/review-action.test.ts b/internal/versioning/review-action.test.ts index cc18ec6..1f13649 100644 --- a/internal/versioning/review-action.test.ts +++ b/internal/versioning/review-action.test.ts @@ -1,3 +1,4 @@ +import { execFileSync, spawnSync } from 'node:child_process' import { readFileSync } from 'node:fs' import { resolve } from 'node:path' import { describe, expect, test } from 'vitest' @@ -46,3 +47,162 @@ describe('Ladon orchestrator manifest', () => { ) }) }) + +// Execute the production shell, including its inner guard, with a fake gh. +// This catches an APPROVE that a later cleanup or failed check cannot undo. +describe('orchestrator approval boundary', () => { + const manifest = parse( + readFileSync( + resolve(import.meta.dirname, '../../ladon/review/action.yml'), + 'utf8', + ), + ) + const setup = manifest.runs.steps.find((s: ActionStep) => s.name === 'Setup') + const arbiter = manifest.runs.steps.find( + (s: ActionStep) => s.name === 'Arbiter', + ) + const reapprove = manifest.runs.steps.find( + (s: ActionStep) => s.name === 'Re-approve dismissed stale approval', + ) + const validate = manifest.runs.steps.find( + (s: ActionStep) => s.name === 'Validate inputs', + ) + + test('defaults compatibly and forwards the control to both actions', () => { + expect(manifest.inputs['auto-approve'].default).toBe('true') + for (const step of [setup, arbiter]) { + expect(step.with['auto-approve']).toBe(`${'$'}{{ inputs.auto-approve }}`) + } + expect(reapprove.if).toBe( + "inputs.auto-approve == 'true' && steps.setup.outputs.reapprove == 'true'", + ) + }) + + test.each(['false', '', 'False', 'off'])( + 'auto-approve=%j prevents every stale reapproval API call even if the step runs', + (value) => { + const result = spawnSync( + 'bash', + ['-c', `gh() { echo 'API CALLED'; }; ${reapprove.run}`], + { + encoding: 'utf8', + env: { + ...process.env, + AUTO_APPROVE: value, + REPO: 'o/r', + PR_NUMBER: '2911', + HEAD_SHA: 'exact-head', + }, + }, + ) + expect(result.status).toBe(1) + expect(result.stdout).not.toContain('API CALLED') + }, + ) + + test('true mode retains exact-head stale reapproval', () => { + const result = spawnSync( + 'bash', + ['-c', `gh() { printf '%s\\n' "$@"; }; ${reapprove.run}`], + { + encoding: 'utf8', + env: { + ...process.env, + AUTO_APPROVE: 'true', + REPO: 'o/r', + PR_NUMBER: '2911', + HEAD_SHA: 'exact-head', + }, + }, + ) + expect(result.status).toBe(0) + expect(result.stdout).toContain('event=APPROVE') + expect(result.stdout).toContain('commit_id=exact-head') + }) + + test.each(['false', 'true', '', 'False', 'disabled'])( + 'validates auto-approve=%j before minting a write token', + (value) => { + const result = spawnSync('bash', ['-c', validate.run], { + encoding: 'utf8', + env: { + ...process.env, + AUTO_APPROVE: value, + CLIENT_ID: '123', + APP_ID: '', + }, + }) + expect(result.status).toBe(['false', 'true'].includes(value) ? 0 : 1) + expect(manifest.runs.steps[0]).toBe(validate) + }, + ) + + test('reviewer tool grants cannot submit approving reviews', () => { + const reviewer = parse( + readFileSync( + resolve(import.meta.dirname, '../../ladon/reviewer/action.yml'), + 'utf8', + ), + ) + const modelSteps = reviewer.runs.steps.filter((s: { uses?: string }) => + s.uses?.startsWith('anthropics/claude-code-action@'), + ) + expect(modelSteps).toHaveLength(2) + const grants = modelSteps.map( + (s: { with: { claude_args: string } }) => s.with.claude_args, + ) + expect(grants[0]).toContain('Bash(gh pr view:*)') + expect(grants.join('\n')).not.toMatch(/Bash\(gh (?:api|pr review)/) + expect(grants.join('\n')).not.toContain('create_review') + expect(grants[1]).toContain( + '--allowedTools "mcp__ladon_findings__finalize_review"', + ) + }) +}) + +describe('audited Ladon dependency pins', () => { + const root = resolve(import.meta.dirname, '../..') + const manifest = parse( + readFileSync(resolve(root, 'ladon/review/action.yml'), 'utf8'), + ) + + test.each(['Setup', 'Reviewer', 'Arbiter'])( + '%s is immutable and matches the local tested runtime', + (name) => { + const step = manifest.runs.steps.find((s: ActionStep) => s.name === name) + const match = + /^adcontextprotocol\/actions\/(ladon\/[a-z]+)@([a-f0-9]{40})$/.exec( + step.uses, + ) + expect(match).not.toBeNull() + if (!match) + throw new Error( + 'Ladon dependencies must be pinned before posting any review', + ) + const [, path, sha] = match + const files = execFileSync( + 'git', + ['ls-tree', '-r', '--name-only', sha, '--', path], + { cwd: root, encoding: 'utf8' }, + ) + .trim() + .split('\n') + .filter( + (file) => !file.includes('.test.') && !file.endsWith('.gitkeep'), + ) + expect(files).toContain(`${path}/action.yml`) + for (const file of files) { + expect( + readFileSync(resolve(root, file), 'utf8'), + `${name}: ${file} differs from its pin; commit and repin the audited runtime`, + ).toBe( + execFileSync('git', ['show', `${sha}:${file}`], { + cwd: root, + encoding: 'utf8', + maxBuffer: 8 * 1024 * 1024, + }), + ) + } + }, + ) +}) diff --git a/ladon/HUMAN-APPROVAL-ADOPTION.md b/ladon/HUMAN-APPROVAL-ADOPTION.md new file mode 100644 index 0000000..f1718a8 --- /dev/null +++ b/ladon/HUMAN-APPROVAL-ADOPTION.md @@ -0,0 +1,207 @@ +# AdCP human approval adoption + +This change provides an opt-in no-approval boundary. **It does not by itself fix +branch protection.** Both consuming workflows must adopt it, existing approvals +and in-flight old runs must be accounted for, and maintainers must audit the +rulesets. Keep the actions PR Draft and auto-merge off during coordination. + +## Incident and audited revisions + +On 2026-09-14, [adcp-client #2911](https://github.com/adcontextprotocol/adcp-client/pull/2911) +merged with only an AAO Secretariat App approval at head +`a6834d67870253674178d099447af36dd6640b11`; `bmilekic` remained requested and had +not reviewed. The PR author enabled auto-merge. REST evidence records review +5200764177 at `17:23:07Z` and merge `8ba12c2ace85a88533ce1d56efd35badea51a97c` +at `17:23:15Z`. Green source CI and an independent agent audit were not human +approval. The incident report records no package release; +[Version Packages #2912](https://github.com/adcontextprotocol/adcp-client/pull/2912) +was confirmed open, Draft, and without auto-merge during this audit. Its +quarantine is outside this change. + +Complete consuming workflows were read at these current-main snapshots: + +| Repository | Main commit | Tree | +| ----------- | ------------------------------------------ | ------------------------------------------ | +| actions | `a64a17ba369122d6b3401f614a31df7b8607f043` | `0029081e46f388904c186c3402a0a4eb9dd523cc` | +| adcp | `36a86d6c4ae5c42e5e2e783d95bdb97b5cb6f614` | `c285d352a02be51e4a787d6208d52230dea81330` | +| adcp-client | `8ba12c2ace85a88533ce1d56efd35badea51a97c` | `1771a8ef743b4ddcd89c836cee39563096dac82e` | + +Sources: [adcp workflow](https://github.com/adcontextprotocol/adcp/blob/36a86d6c4ae5c42e5e2e783d95bdb97b5cb6f614/.github/workflows/ai-review.yml), +[adcp-client workflow](https://github.com/adcontextprotocol/adcp-client/blob/8ba12c2ace85a88533ce1d56efd35badea51a97c/.github/workflows/ai-review.yml). +Both use `pull_request_target`, a trusted base checkout, `code_review`, the +review-workflow-modification gate, and `@ladon/review/v1`. The client workflow +explicitly says App reviews count toward “1 review required.” Neither supplies +an approval control at the audited revision. + +At audit time, review/setup/arbiter floating v1 tags resolved to +`ab7c8394702dad2244b48143c78b6651cd5215ed`; reviewer v1 resolved to the actions +base commit above. No tag was moved by this work. + +## Approval paths and the new boundary + +1. The arbiter maps an `approve` model outcome to an `APPROVE` review API call. + Its own-PR 422 fallback tries approval **before** posting COMMENT; a fallback + is not a no-approval guarantee. Disabled mode converts the decision to + `comment` before rendering its Ladon body/marker and also clamps APPROVE to + COMMENT at the final API boundary, before the first write. +2. Setup can signal `reapprove=true` on an empty/trivial delta or pure rebase, + using a previous Ladon `approve` marker plus aggregate `REVIEW_REQUIRED`. + The orchestrator then POSTs `APPROVE` directly, bypassing the arbiter's team, + gated-path and high-risk checks. The compatible mode retains that legacy + behavior; it must not be used as a human-only gate. Disabled mode suppresses + renewal in setup, the orchestrator condition, and the shell itself. It + re-reviews the full surface on no-delta pushes so a prior failed/escalated + result cannot silently become a successful skip. +3. The reviewer has inline-comment and finding-persistence tools, read-only gh + commands, and one finalization-only retry. It has no permitted top-level + review/gh write tool. Incomplete review state fails before the arbiter runs. + Workflow-modification notices use COMMENT. Bot-author/self-trigger skips + never produce approval. + +Set `auto-approve: 'false'` in the **trusted consuming workflow**, not in PR +prose or LADON.md. The manifest default stays `'true'` for existing consumers. +Explicit empty, malformed, or unsupported values fail before posting. The +same input is available on setup and arbiter for direct users; they must apply +it consistently and remove/guard any approval code in their own orchestrator. + +In disabled mode: + +| Result | Review event | Action result | +| ----------------------------------------------- | -------------------------------------------- | -------------------------------------------------- | +| Clean or nonblocking findings | COMMENT, with Ladon body and decision marker | Success after complete review | +| Critical/high findings | REQUEST_CHANGES | Failure | +| Governance/high-risk escalation | COMMENT, escalation label/reviewer request | Failure | +| Incomplete reviewer, API failure, invalid input | No approving review | Failure | +| Existing configured author/event skip | No approving review | Existing skip behavior; **not evidence of review** | + +Known blocking policy rows are enforced in code in disabled mode even if the +model returns approve/comment. An escalation remains failed until its reason +is resolved; a human approval alone does not turn a destructive-change +escalation green. Maintainers must explicitly handle an intentional high-risk +change under their audited human exception procedure, not add +`continue-on-error` or an unconditional successful check. The original mode's +review/check semantics remain compatible. + +The orchestrator pins setup/arbiter to an immutable implementation commit with +this control and reviewer to the audited base commit. Otherwise an old nested +floating action could ignore `auto-approve` and create approval before any +post-action capability check. Future sub-action updates require an explicit +orchestrator pin update, tests, and review. Preserve the referenced commits +when landing this series; do not discard the implementation commit while +retaining its pin. + +## Auto-merge race and limits + +An approving review is a durable GitHub write. Posting APPROVE and subsequently +dismissing it or failing a check leaves a merge window, especially if the +review job is optional, skipped, or an older successful check is accepted. +This implementation never emits that initial approval in disabled mode. Tests +inspect every review API call and execute the reapproval shell with a fake gh; +cleanup is not the control. + +COMMENT does **not** erase an existing active approval. Nor can this action +stop a run using an older workflow/action revision, another App, a machine +account typed as `User`, or a bypass actor. A required job that skips may count +as successful; a path-filtered workflow may remain pending. See +[GitHub's required-check semantics](https://docs.github.com/en/pull-requests/how-tos/merge-and-close-pull-requests/troubleshooting-required-status-checks). +The legacy `hasHumanApproval` helper is a supplemental gated-path check, not an +exact-head, authorized-human gate: it does not verify latest per-user review, +commit, author exclusion, or reviewer authority. Do not use it as that gate. + +## Coordinated landing order + +1. **Hold merges during migration.** Human maintainers keep affected PRs Draft + or auto-merge disabled, identify/cancel/drain old approval-capable runs, and + inspect active Ladon approvals. Dismiss obsolete bot approvals while the + hold is in place. Never depend on racing a cleanup against auto-merge. +2. Obtain exact-head CI and independent security review for the actions Draft + PR, then explicit human review. Record base/head/tree, immutable sub-action + pins, tests, and review evidence in the PR. No publishing or tag movement + is authorized by this preparation. +3. Open **separate human-reviewed follow-up PRs in adcp and adcp-client**. In + each `.github/workflows/ai-review.yml`, pin `Run Ladon` to the human-reviewed + actions PR head SHA (replace `REVIEWED_ACTIONS_SHA` below), and add the input: + + ```yaml + - name: Run Ladon + if: steps.workflow-mod.outputs.modified != 'true' + uses: adcontextprotocol/actions/ladon/review@REVIEWED_ACTIONS_SHA + with: + auto-approve: "false" + # Retain each repository's existing credentials, model, and bot list. + ``` + + Remove the client's claim that Ladon satisfies “1 review required.” State + in both workflows that Ladon provides automated findings and human approval + is separate. Preserve `pull_request_target`, base-SHA checkout, `code_review`, + permissions and the workflow-modification gate. Preserve each bot list + (`aao-release-bot[bot]` in adcp; `aao-ipr-bot[bot]` in adcp-client). + Do not relax the gate to test the new workflow against its own PR head. + Pure workflow edits are path-filtered; mixed PRs get only the base + workflow's COMMENT and skip Ladon. Neither follow-up can self-approve. + If filtered required checks stay pending, a maintainer must resolve that + through the audited human procedure; do not manufacture a green result. + +4. **Land both consumer PRs with real human approval before moving + `ladon/review/v1`.** Pinning the reviewed implementation makes the new mode + effective without waiting for a tag. Confirm both default-branch workflows + contain the pin/input and inspect runs at those revisions. Keep old runs + and pre-existing approvals under the migration hold until accounted for. +5. Complete the ruleset audit below. Only after both consumers have landed and + verification is recorded may maintainers authorize merging/publishing the + actions change. **Merging this PR to actions/main automatically invokes + `merge-publish.yml`**, which advances affected patch and floating major tags; + `manual-version-publish.yml` can also move them. Thus merging the actions PR + first would violate the required order even without a manual tag command. + No tag, merge, deployment, or publication is performed in this task. +6. Keep the consumer SHA pins for reproducibility, or human-review a later + switch to the promoted `@ladon/review/v1`, retaining `auto-approve: 'false'`. + A later workflow change is subject to the same human-only modification gate. + +## Required manual ruleset audit + +Read-only ruleset inspection was available. Both repositories inherit active +organization ruleset `8519441` (one approval, CODEOWNERS required, admin/role +and integration bypasses). Their active repository rulesets `15545291` (adcp) +and `15545837` (client) additionally dismiss stale approvals and require +last-push approval. These are **approval-count rules**, not proof of a human. +The visible required-check lists include source CI and IPR, but neither +`code_review` nor a separate human-review check. The classic branch-protection +API returned **403** for both; therefore this is not a complete effective-rule +audit. No settings were changed. + +An authorized maintainer must inspect repository **and inherited organization** +rulesets, classic protection, CODEOWNERS coverage, App permissions and all +bypass actors. Confirm which exact check contexts and expected App identities +are required, including skipped/path-filtered cases. Record the actual rules +and verification evidence; requesting a reviewer is not approval. + +If a human-only approval requirement is not already independently enforced, +add a trusted required check (for example `Human review / exact head`) with +these properties before lifting the hold: + +- Read live PR head and paginated reviews using trusted base code. Count only + the latest effective non-dismissed approval from an authorized maintainer or + required owner at that exact head; exclude the PR author, all Apps/bots and + explicitly identified automation accounts, including those typed as User. +- Verify current reviewer authority and CODEOWNERS/required-team policy; fail + closed on unknown identity, permissions, incomplete data, or API failure. +- Recompute on synchronize, ready-for-review, review submission/dismissal and + other policy-affecting events. Tie status to the live head, reject stale + workers, and reset on head/approval changes. Review-only events must not + execute PR-head code or expose write credentials to it. +- Publish a uniquely named required status/check from a trusted producer, + configure the expected App where supported, and cover workflow-modifying, + bot-authored, fork, rebase, and merge-queue cases. Skips must never mean + “human approved.” Audit bypasses and concurrent review dismissal behavior. +- Verify on controlled PRs: bot-only approval stays blocked, requested-but-no- + review stays blocked, dismissed/old-head/self approval stays blocked, an + authorized exact-head human approval lifts only the human gate, and blocking + Ladon findings still prevent merging under the repository's policy. + +[GitHub documents required reviews, bypass behavior, and expected check sources](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches). +This separate gate must itself be installed through human-reviewed workflow +and ruleset changes. The shared action cannot supply repository-wide human +identity enforcement with its current workflow permissions and event coverage. +Do not report branch protection fixed until consumer adoption and the +applicable tag/ruleset changes are actually complete. diff --git a/ladon/INSTALL.md b/ladon/INSTALL.md index a7058db..7354383 100644 --- a/ladon/INSTALL.md +++ b/ladon/INSTALL.md @@ -15,7 +15,7 @@ Ladon posts reviews under the **AAO Secretariat** GitHub App, using Claude for t review itself. Before the workflow can run: 1. **Install the AAO Secretariat App on the repo.** Reviews post under the App - identity so they count toward a "1 review required" branch-protection rule. + identity so automated findings are attributable to Ladon. Ask an org admin if it isn't already installed. 2. **Add three repository secrets** (Settings → Secrets and variables → Actions): @@ -25,13 +25,20 @@ review itself. Before the workflow can run: | `SECRETARIAT_APP_PRIVATE_KEY` | The App's private key (PEM) | | `ANTHROPIC_API_KEY` | Anthropic API key Ladon reviews with | -3. **(Optional) Branch protection.** If `main` requires an approving review, - Ladon's App-authored `approve` satisfies it; its `request-changes` blocks the - merge until addressed. Verify that the App can dismiss reviews on the - protected branch; when dismissal restrictions are enabled, add the App to the - branch rule's or ruleset's allowed dismissal actors. Without that access, - Ladon escalates with explicit instructions for an authorized maintainer to - dismiss the stale review. +3. **Choose the approval policy explicitly.** Existing consumers default to + `auto-approve: 'true'`, which allows App approvals to satisfy GitHub's review + count. For human-only repositories set `auto-approve: 'false'` as shown + below: clean reviews become COMMENT, blocking findings still request changes + and fail, and escalations remain failed pending human resolution. This input + does not itself enforce who may satisfy branch protection. Follow the + [coordinated adoption and ruleset audit](./HUMAN-APPROVAL-ADOPTION.md), including + existing approvals and old in-flight runs, before relying on the human gate. + If the App cannot dismiss its superseded change requests, an authorized + maintainer must resolve them; do not ignore a failed check. + +The template below opts into findings without approving reviews. Use a reviewed +immutable action SHA during migration; an older `ladon/review/v1` can ignore the +new input. See the adoption order before using the floating tag. ## Step 1: add the workflow @@ -135,6 +142,7 @@ jobs: if: steps.workflow-mod.outputs.modified != 'true' uses: adcontextprotocol/actions/ladon/review@ladon/review/v1 with: + auto-approve: "false" anthropic-api-key: ${{ secrets.ANTHROPIC_API_KEY }} app-id: ${{ secrets.SECRETARIAT_APP_ID }} app-private-key: ${{ secrets.SECRETARIAT_APP_PRIVATE_KEY }} diff --git a/ladon/README.md b/ladon/README.md index 7d00ff0..c3bc210 100644 --- a/ladon/README.md +++ b/ladon/README.md @@ -24,6 +24,14 @@ AdCP Working Group. Ladon does the reviewing; it posts under the AAO Secretariat bot identity (the App whose credentials the consuming workflow supplies). See `reviewer/rules/voice.md`. +## Human-only approval + +Set `auto-approve: 'false'` on the orchestrator to post findings without ever +submitting an approving review, including stale reapproval. The default remains +`'true'` for compatibility. Blocking findings and escalation still fail in the +new mode. See [the adoption plan](./HUMAN-APPROVAL-ADOPTION.md) for consumer pins, +auto-merge races, and the separate required human-review/ruleset audit. + ## Per-repo configuration Repo-specific tuning lives in a `LADON.md` file at each consuming repo's root diff --git a/ladon/review/action.yml b/ladon/review/action.yml index 188b616..66ffd36 100644 --- a/ladon/review/action.yml +++ b/ladon/review/action.yml @@ -3,6 +3,10 @@ description: 'Runs the Ladon review flow: setup → reviewer → arbiter.' author: 'Ad Context Protocol' inputs: + auto-approve: + description: 'Whether Ladon may submit APPROVE (true or false). Set false for findings without approving reviews; blocking findings and escalations fail the action.' + required: false + default: 'true' anthropic-api-key: description: 'Anthropic API key' required: true @@ -66,8 +70,17 @@ runs: steps: - name: Validate inputs shell: bash + env: + CLIENT_ID: ${{ inputs.client-id }} + APP_ID: ${{ inputs.app-id }} + AUTO_APPROVE: ${{ inputs.auto-approve }} run: | - if [[ -z "${{ inputs.client-id }}" && -z "${{ inputs.app-id }}" ]]; then + set -euo pipefail + if [[ "$AUTO_APPROVE" != 'true' && "$AUTO_APPROVE" != 'false' ]]; then + echo '::error::auto-approve must be exactly "true" or "false"' + exit 1 + fi + if [[ -z "$CLIENT_ID" && -z "$APP_ID" ]]; then echo "::error::Either 'client-id' or 'app-id' must be provided" exit 1 fi @@ -79,12 +92,16 @@ runs: client-id: ${{ inputs.client-id || inputs.app-id }} private-key: ${{ inputs.app-private-key }} + # Pin the audited action tree: an older floating setup/arbiter can ignore + # auto-approve and write an approval before any capability check could run. + # Update these pins explicitly when promoting a reviewed sub-action change. - name: Setup id: setup - uses: adcontextprotocol/actions/ladon/setup@ladon/setup/v1 + uses: adcontextprotocol/actions/ladon/setup@dc93cb8ec91206c7a8097638a7e559bf58a08aee with: github-token: ${{ steps.app-token.outputs.token }} ladon-bot-login: ${{ steps.app-token.outputs.app-slug }}[bot] + auto-approve: ${{ inputs.auto-approve }} protected-branches: ${{ inputs.protected-branches }} no-auto-approve-teams: ${{ inputs.no-auto-approve-teams }} escalation-reviewers: ${{ inputs.escalation-reviewers }} @@ -103,21 +120,28 @@ runs: echo "should-run=false skip-reason=${{ steps.setup.outputs.skip-reason }}" >> "$GITHUB_STEP_SUMMARY" - name: Re-approve dismissed stale approval - if: steps.setup.outputs.reapprove == 'true' + if: inputs.auto-approve == 'true' && steps.setup.outputs.reapprove == 'true' shell: bash env: GH_TOKEN: ${{ steps.app-token.outputs.token }} + AUTO_APPROVE: ${{ inputs.auto-approve }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ steps.setup.outputs.pr-number }} + HEAD_SHA: ${{ steps.setup.outputs.head-sha }} run: | - gh api repos/${{ github.repository }}/pulls/${{ steps.setup.outputs.pr-number }}/reviews \ + set -euo pipefail + # Defense in depth if this step is reused or its condition changes. + [[ "$AUTO_APPROVE" == 'true' ]] || exit 1 + gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews" \ --method POST \ -f body='Re-approval: no reviewable delta since the prior approval; re-submitting after stale-review dismissal.' \ -f event='APPROVE' \ - -f commit_id='${{ steps.setup.outputs.head-sha }}' + -f commit_id="$HEAD_SHA" - name: Reviewer if: steps.setup.outputs.should-run == 'true' id: reviewer - uses: adcontextprotocol/actions/ladon/reviewer@ladon/reviewer/v1 + uses: adcontextprotocol/actions/ladon/reviewer@a64a17ba369122d6b3401f614a31df7b8607f043 with: anthropic-api-key: ${{ inputs.anthropic-api-key }} github-token: ${{ steps.app-token.outputs.token }} @@ -138,11 +162,12 @@ runs: - name: Arbiter if: steps.setup.outputs.should-run == 'true' id: arbiter - uses: adcontextprotocol/actions/ladon/arbiter@ladon/arbiter/v1 + uses: adcontextprotocol/actions/ladon/arbiter@dc93cb8ec91206c7a8097638a7e559bf58a08aee with: anthropic-api-key: ${{ inputs.anthropic-api-key }} github-token: ${{ steps.app-token.outputs.token }} ladon-bot-login: ${{ steps.app-token.outputs.app-slug }}[bot] + auto-approve: ${{ inputs.auto-approve }} findings-json: ${{ steps.reviewer.outputs.findings-json }} ladon-md-body: ${{ steps.setup.outputs.ladon-md-body }} high-risk: ${{ steps.setup.outputs.high-risk }} From 292a0da93b25c3e9ecfcf5a05763a4206c472c3a Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 14 Sep 2026 17:59:41 +0000 Subject: [PATCH 03/11] fix(ladon): preserve blocking policy across review passes --- ladon/arbiter/dist/index.js | 2 +- ladon/arbiter/src/enforce.test.ts | 5 ++++ ladon/arbiter/src/enforce.ts | 4 ++- ladon/setup/dist/index.js | 24 +++++++---------- ladon/setup/src/high-risk.test.ts | 27 +++++++++++++++++++ ladon/setup/src/high-risk.ts | 17 ++++++------ ladon/setup/src/index.ts | 15 +++-------- .../setup/src/no-approval.integration.test.ts | 18 +++++++++++++ 8 files changed, 77 insertions(+), 35 deletions(-) diff --git a/ladon/arbiter/dist/index.js b/ladon/arbiter/dist/index.js index 703aae5..21f5fe9 100644 --- a/ladon/arbiter/dist/index.js +++ b/ladon/arbiter/dist/index.js @@ -47508,7 +47508,7 @@ function enforceDecisionGuards(decision, ctx) { reasons.push('Medium finding in a sensitive category requires human review.'); } if (ctx.highRisk && - ctx.highRiskReasons?.some((r) => r.includes('(modified)')) && + ctx.highRiskReasons?.some((r) => r.includes('(modified)') || r.includes('(renamed)')) && mediums.length > 0) { reasons.push('Medium finding with a modified high-risk path requires human review.'); } diff --git a/ladon/arbiter/src/enforce.test.ts b/ladon/arbiter/src/enforce.test.ts index 420d84a..c1a52c1 100644 --- a/ladon/arbiter/src/enforce.test.ts +++ b/ladon/arbiter/src/enforce.test.ts @@ -280,6 +280,11 @@ test.each([ highRiskReasons: ['src/a.ts (modified)'], expected: 'escalate', }, + { + highRisk: true, + highRiskReasons: ['src/a.ts (renamed)'], + expected: 'escalate', + }, { highRisk: true, highRiskReasons: ['src/a.ts (added)'], diff --git a/ladon/arbiter/src/enforce.ts b/ladon/arbiter/src/enforce.ts index 888d5ee..99682e0 100644 --- a/ladon/arbiter/src/enforce.ts +++ b/ladon/arbiter/src/enforce.ts @@ -56,7 +56,9 @@ export function enforceDecisionGuards( } if ( ctx.highRisk && - ctx.highRiskReasons?.some((r) => r.includes('(modified)')) && + ctx.highRiskReasons?.some( + (r) => r.includes('(modified)') || r.includes('(renamed)'), + ) && mediums.length > 0 ) { reasons.push( diff --git a/ladon/setup/dist/index.js b/ladon/setup/dist/index.js index 5247aea..44fecc8 100644 --- a/ladon/setup/dist/index.js +++ b/ladon/setup/dist/index.js @@ -39589,21 +39589,21 @@ function boundReasonsForActionInput(reasons) { ACTION_INPUT_REASON_BUDGET_BYTES) { return reasons; } + // These diagnostics also carry facts used by arbiter escalation policy. + // Preserve every change kind even when its individual reason is omitted. + const kinds = ['added', 'modified', 'deleted', 'renamed'].filter((kind) => reasons.some((reason) => reason.includes(`(${kind})`))); + const omittedMessage = (count) => `… ${count} additional path matches omitted; change kinds in the full match set: ${kinds.map((kind) => `(${kind})`).join(', ')}`; const bounded = []; for (const reason of reasons) { const omitted = reasons.length - bounded.length - 1; - const candidate = [ - ...bounded, - reason, - `… ${omitted} additional path matches omitted`, - ]; + const candidate = [...bounded, reason, omittedMessage(omitted)]; if (Buffer.byteLength(JSON.stringify(candidate), 'utf8') > ACTION_INPUT_REASON_BUDGET_BYTES) { break; } bounded.push(reason); } - bounded.push(`… ${reasons.length - bounded.length} additional path matches omitted`); + bounded.push(omittedMessage(reasons.length - bounded.length)); return bounded; } /** @@ -40027,7 +40027,10 @@ async function main() { let deltaFiles = surfacePaths; let isPureRebase = false; const ladonBotLogin = getInput('ladon-bot-login') || 'ladon[bot]'; - if (eventAction === 'synchronize') { + // Findings-only mode uses the full PR on every pass. Otherwise a nonempty + // delta could omit an unresolved medium finding that made the prior check + // fail (the reviewer only rechecks prior mediums within its delta). + if (eventAction === 'synchronize' && autoApprove) { try { const reviews = await octokit.paginate(octokit.rest.pulls.listReviews, { owner, @@ -40161,13 +40164,6 @@ async function main() { setOutput('skip-reason', decision.skipReason ?? ''); return; } - // With approvals disabled, a no-delta push must recheck the full surface: - // a previous failure/escalation cannot become a green check just by rebasing - // or changing a trivial file. Never reuse a dismissed approval as a verdict. - if (!autoApprove && - (isPureRebase || deltaFilesAfterTrivialFilter.length === 0)) { - deltaFiles = surfacePaths; - } // High-risk / gated-path evaluation runs on the PR surface. Change kinds // come from the pulls.listFiles response already fetched above — no extra // API call and, crucially, no fetch of the PR head. diff --git a/ladon/setup/src/high-risk.test.ts b/ladon/setup/src/high-risk.test.ts index be337d2..68882b2 100644 --- a/ladon/setup/src/high-risk.test.ts +++ b/ladon/setup/src/high-risk.test.ts @@ -86,3 +86,30 @@ describe('boundReasonsForActionInput', () => { expect(bounded.length).toBeLessThan(reasons.length) }) }) + +test('retains policy-relevant change kinds beyond the diagnostic budget', () => { + const reasons = [ + ...Array.from( + { length: 1000 }, + (_, i) => `src/new-${i}.ts (added) matches src/**`, + ), + 'src/security.ts (deleted) matches src/**', + 'src/auth.ts (modified) matches src/**', + ] + const bounded = boundReasonsForActionInput(reasons) + expect( + Buffer.byteLength(JSON.stringify(bounded), 'utf8'), + ).toBeLessThanOrEqual(32 * 1024) + expect(bounded.some((reason) => reason.includes('(deleted)'))).toBe(true) + expect(bounded.some((reason) => reason.includes('(modified)'))).toBe(true) +}) + +test('retains change kinds even if one reason alone exceeds the budget', () => { + const bounded = boundReasonsForActionInput([ + `${'x'.repeat(40_000)} (deleted) matches src/**`, + ]) + expect( + Buffer.byteLength(JSON.stringify(bounded), 'utf8'), + ).toBeLessThanOrEqual(32 * 1024) + expect(bounded.some((reason) => reason.includes('(deleted)'))).toBe(true) +}) diff --git a/ladon/setup/src/high-risk.ts b/ladon/setup/src/high-risk.ts index 60fd8fe..da97c11 100644 --- a/ladon/setup/src/high-risk.ts +++ b/ladon/setup/src/high-risk.ts @@ -30,14 +30,17 @@ export function boundReasonsForActionInput(reasons: string[]): string[] { return reasons } + // These diagnostics also carry facts used by arbiter escalation policy. + // Preserve every change kind even when its individual reason is omitted. + const kinds = ['added', 'modified', 'deleted', 'renamed'].filter((kind) => + reasons.some((reason) => reason.includes(`(${kind})`)), + ) + const omittedMessage = (count: number) => + `… ${count} additional path matches omitted; change kinds in the full match set: ${kinds.map((kind) => `(${kind})`).join(', ')}` const bounded: string[] = [] for (const reason of reasons) { const omitted = reasons.length - bounded.length - 1 - const candidate = [ - ...bounded, - reason, - `… ${omitted} additional path matches omitted`, - ] + const candidate = [...bounded, reason, omittedMessage(omitted)] if ( Buffer.byteLength(JSON.stringify(candidate), 'utf8') > ACTION_INPUT_REASON_BUDGET_BYTES @@ -47,9 +50,7 @@ export function boundReasonsForActionInput(reasons: string[]): string[] { bounded.push(reason) } - bounded.push( - `… ${reasons.length - bounded.length} additional path matches omitted`, - ) + bounded.push(omittedMessage(reasons.length - bounded.length)) return bounded } diff --git a/ladon/setup/src/index.ts b/ladon/setup/src/index.ts index 530eff5..8533fe2 100644 --- a/ladon/setup/src/index.ts +++ b/ladon/setup/src/index.ts @@ -264,7 +264,10 @@ async function main(): Promise { const ladonBotLogin = core.getInput('ladon-bot-login') || 'ladon[bot]' - if (eventAction === 'synchronize') { + // Findings-only mode uses the full PR on every pass. Otherwise a nonempty + // delta could omit an unresolved medium finding that made the prior check + // fail (the reviewer only rechecks prior mediums within its delta). + if (eventAction === 'synchronize' && autoApprove) { try { const reviews = await octokit.paginate(octokit.rest.pulls.listReviews, { owner, @@ -409,16 +412,6 @@ async function main(): Promise { return } - // With approvals disabled, a no-delta push must recheck the full surface: - // a previous failure/escalation cannot become a green check just by rebasing - // or changing a trivial file. Never reuse a dismissed approval as a verdict. - if ( - !autoApprove && - (isPureRebase || deltaFilesAfterTrivialFilter.length === 0) - ) { - deltaFiles = surfacePaths - } - // High-risk / gated-path evaluation runs on the PR surface. Change kinds // come from the pulls.listFiles response already fetched above — no extra // API call and, crucially, no fetch of the PR head. diff --git a/ladon/setup/src/no-approval.integration.test.ts b/ladon/setup/src/no-approval.integration.test.ts index 56e1644..06b75ce 100644 --- a/ladon/setup/src/no-approval.integration.test.ts +++ b/ladon/setup/src/no-approval.integration.test.ts @@ -172,6 +172,24 @@ describe('setup stale-approval integration', () => { }, ) + test('an unrelated nontrivial push still reviews the full PR in disabled mode', async () => { + fixture.octokit.paginate.mockResolvedValueOnce([ + { filename: 'src/auth.ts', status: 'modified' }, + { filename: 'src/other.ts', status: 'modified' }, + ]) + fixture.octokit.rest.repos.compareCommitsWithBasehead.mockResolvedValue({ + data: { files: [{ filename: 'src/other.ts' }] }, + }) + await run() + expect(fixture.outputs['should-run']).toBe('true') + expect( + fixture.octokit.rest.repos.compareCommitsWithBasehead, + ).not.toHaveBeenCalled() + expect(fixture.outputs['diff-delta-path']).toBe( + fixture.outputs['diff-full-path'], + ) + }) + test('normal opened review runs with approval disabled', async () => { fixture.context.payload.action = 'opened' await run() From 85284cbc297875aa04b2d5afcf7ae833b29f329c Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 14 Sep 2026 18:00:06 +0000 Subject: [PATCH 04/11] fix(ladon): pin reviewed policy-preserving actions --- ladon/HUMAN-APPROVAL-ADOPTION.md | 9 ++++++--- ladon/review/action.yml | 4 ++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/ladon/HUMAN-APPROVAL-ADOPTION.md b/ladon/HUMAN-APPROVAL-ADOPTION.md index f1718a8..4217630 100644 --- a/ladon/HUMAN-APPROVAL-ADOPTION.md +++ b/ladon/HUMAN-APPROVAL-ADOPTION.md @@ -50,8 +50,9 @@ base commit above. No tag was moved by this work. gated-path and high-risk checks. The compatible mode retains that legacy behavior; it must not be used as a human-only gate. Disabled mode suppresses renewal in setup, the orchestrator condition, and the shell itself. It - re-reviews the full surface on no-delta pushes so a prior failed/escalated - result cannot silently become a successful skip. + reviews the full PR surface on every run so a prior failed/escalated result + cannot silently become a successful skip or lose an unresolved medium finding + outside an incremental delta. This costs more review work than legacy mode. 3. The reviewer has inline-comment and finding-persistence tools, read-only gh commands, and one finalization-only retry. It has no permitted top-level review/gh write tool. Incomplete review state fails before the arbiter runs. @@ -75,7 +76,9 @@ In disabled mode: | Existing configured author/event skip | No approving review | Existing skip behavior; **not evidence of review** | Known blocking policy rows are enforced in code in disabled mode even if the -model returns approve/comment. An escalation remains failed until its reason +model returns approve/comment. Renames count as modifications, and bounded +path diagnostics retain change-kind facts even when individual reasons exceed +the input budget. An escalation remains failed until its reason is resolved; a human approval alone does not turn a destructive-change escalation green. Maintainers must explicitly handle an intentional high-risk change under their audited human exception procedure, not add diff --git a/ladon/review/action.yml b/ladon/review/action.yml index 66ffd36..ed19be3 100644 --- a/ladon/review/action.yml +++ b/ladon/review/action.yml @@ -97,7 +97,7 @@ runs: # Update these pins explicitly when promoting a reviewed sub-action change. - name: Setup id: setup - uses: adcontextprotocol/actions/ladon/setup@dc93cb8ec91206c7a8097638a7e559bf58a08aee + uses: adcontextprotocol/actions/ladon/setup@292a0da93b25c3e9ecfcf5a05763a4206c472c3a with: github-token: ${{ steps.app-token.outputs.token }} ladon-bot-login: ${{ steps.app-token.outputs.app-slug }}[bot] @@ -162,7 +162,7 @@ runs: - name: Arbiter if: steps.setup.outputs.should-run == 'true' id: arbiter - uses: adcontextprotocol/actions/ladon/arbiter@dc93cb8ec91206c7a8097638a7e559bf58a08aee + uses: adcontextprotocol/actions/ladon/arbiter@292a0da93b25c3e9ecfcf5a05763a4206c472c3a with: anthropic-api-key: ${{ inputs.anthropic-api-key }} github-token: ${{ steps.app-token.outputs.token }} From 36494984c6f6bc9640186a75829644e9097a60a2 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 14 Sep 2026 18:12:01 +0000 Subject: [PATCH 05/11] docs(ladon): record repeated human-gate bypasses and administrator audit --- ladon/HUMAN-APPROVAL-ADOPTION.md | 92 ++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/ladon/HUMAN-APPROVAL-ADOPTION.md b/ladon/HUMAN-APPROVAL-ADOPTION.md index 4217630..8d7c43e 100644 --- a/ladon/HUMAN-APPROVAL-ADOPTION.md +++ b/ladon/HUMAN-APPROVAL-ADOPTION.md @@ -37,6 +37,57 @@ At audit time, review/setup/arbiter floating v1 tags resolved to `ab7c8394702dad2244b48143c78b6651cd5215ed`; reviewer v1 resolved to the actions base commit above. No tag was moved by this work. +## Follow-up incident evidence and global hold (2026-09-14 UTC) + +The original snapshots above are historical. Consumer adoption was refreshed to +adcp `1467e46117e8d329f4116e548e9290be75d8e3b3` (tree +`023fac83c8812e6148519ba8a4cfe2a49fbb3070`) and client +`55829081dae2673f73615af2a4f6f029cddff813` (tree +`75aa2d649b3234643ec4619ea7b65a8b86d44320`). Existing open PR evidence, including +adcp #7450/#7462, remains on its recorded older base until independently +refreshed; this change does not recompute their merge trees. + +[adcp #7521](https://github.com/adcontextprotocol/adcp/pull/7521) demonstrates a +**second, distinct bypass**. Base was +`36a86d6c4ae5c42e5e2e783d95bdb97b5cb6f614`; head was +`e680c97e05e69cf3d7b6bf60aed07c42657c420c`. Its only review, Secretariat +`5200346889`, was COMMENTED at `16:38:41Z` with an explicit Ladon human/CODEOWNERS +escalation for modified +`static/schemas/source/compliance/comply-test-controller-{request,response}.json` +and `REVIEW_REQUIRED`. There were **zero APPROVED reviews**. Author `bokelley` +merged it himself at `18:02:02Z` as +`1467e46117e8d329f4116e548e9290be75d8e3b3`; final `auto_merge` was null. +An escalation comment or optional check cannot stop an author with effective +merge permission. The precise bypass rule still requires administrator audit. + +[adcp-client #2913](https://github.com/adcontextprotocol/adcp-client/pull/2913) +had only bot approval at exact head +`bfacfeb66178d161254830faef48f36a5a6a629e`: Secretariat review `5201034616` +APPROVED at `17:51:07Z`, while `bmilekic` remained requested without reviewing. +After the coordinator disabled auto-merge and placed it Draft, author `bokelley` +re-enabled auto-merge at `17:57:24Z`. It merged at `17:59:28Z` as +`55829081dae2673f73615af2a4f6f029cddff813` (base +`8ba12c2ace85a88533ce1d56efd35badea51a97c`). Secretariat review `5201137108` +APPROVED that same head at `18:01:40Z`, **after merge**. The later approval +cannot supply human evidence or explain the earlier merge; it confirms older +approval-capable runs can continue writing. Draft/auto-off comments are +advisory against an author/admin and are not a protection boundary. + +Generated #2912 advanced to `1be6b25e8083601f8bf08f1b701435181f33215f` on +base `55829081dae2673f73615af2a4f6f029cddff813`, and was still Draft/auto-off. +Coordinator evidence reports npm dist-tags unchanged at rc.36 / latest 13.0.4, +with no new package published. **Maintain a global release/merge hold** until +both human-reviewed immutable consumer pins land and an authorized human +configures and validates an actually required, trusted exact-head human gate. +PR #29 alone cannot prevent manual author/admin bypass or older runs. + +Consumer runtime pins stay at reviewed orchestrator +`85284cbc297875aa04b2d5afcf7ae833b29f329c`, nested setup/arbiter +`292a0da93b25c3e9ecfcf5a05763a4206c472c3a`, and reviewer +`a64a17ba369122d6b3401f614a31df7b8607f043`. Documentation-only updates to this +PR do not require moving those runtime pins. The consumer configuration tests +reject missing approval inputs; the shared compatible default remains true. + ## Approval paths and the new boundary 1. The arbiter maps an `approve` model outcome to an `APPROVE` review API call. @@ -208,3 +259,44 @@ and ruleset changes. The shared action cannot supply repository-wide human identity enforcement with its current workflow permissions and event coverage. Do not report branch protection fixed until consumer adoption and the applicable tag/ruleset changes are actually complete. + +### Administrator action and controlled validation after #7521/#2913 + +An explicitly authorized administrator must export the effective rules for +main from repository Settings → Rules → Rulesets, inherited organization +rules, classic protection, CODEOWNERS and every role/App/direct-push bypass. +Determine the exact permission that allowed #7521's author to merge without any +approval. Install the separately human-reviewed trusted producer and require +`Human review / exact head` from its specific App identity; require blocking +Ladon results under an explicit workflow-modification/high-risk exception +policy as well. The new consumer `Ladon approval policy` CI job is only a +configuration/regression check, not this human gate. + +The human gate must exclude the PR author, verify **current** collaborator and +CODEOWNER/team authority, and reject bots/Apps and known automation accounts +masquerading as users. Make it non-bypassable by ordinary authors, including +the author role implicated by #7521. Remove broad role/App bypasses or confine +any emergency override to an independently controlled, logged procedure; +prevent direct pushes and alternative merge entrypoints from bypassing it. +No settings change is authorized by this preparation or by green CI. + +Recompute and invalidate on head, review/dismissal, base/CODEOWNERS and +membership/permission changes. Serialize workers and re-read live head, +review state and authority before publishing; use native stale-dismissal and +last-push rules too. Status webhooks are asynchronous: do not assert atomic +review-dismissal/merge safety without evidence. Retain the operational hold +and an authorized trusted merge procedure if that race is not closed. + +Start controlled validation on a non-production branch with matching effective +protection and no deployment/release hooks. Actual merge attempts require +separate explicit administrator authority. Prove rejection under ordinary +author credentials for no review, bot-only approval, COMMENT-only escalation +(#7521), requested-but-absent review, self/old-head/dismissed approval, lost +collaborator/CODEOWNER authority, missing/skipped/failed status, API failure, +stale worker, fork, workflow modification, rebase and merge queue. Exercise +concurrent push, dismissal and auto-merge. A current-head authorized non-author +human approval satisfies only the human gate; blocking Ladon findings must +remain blocking. Then verify actual main's effective rules and producer App; +a sandbox-only result does not prove main enforcement. Record rule exports, +exact heads/statuses/reviewer authority, denied merge attempts and a named +administrator's acceptance before lifting the global hold or promoting tags. From 02db55f54f39f93c683c2031887b735aea0ecba2 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 14 Sep 2026 18:15:05 +0000 Subject: [PATCH 06/11] fix(ladon)!: require explicit opt-in for approving reviews --- internal/versioning/review-action.test.ts | 89 +++++++++++++++---- ladon/arbiter/action.yml | 2 +- ladon/arbiter/dist/index.js | 6 +- ladon/arbiter/src/enforce.test.ts | 12 ++- ladon/arbiter/src/enforce.ts | 4 +- .../src/no-approval.integration.test.ts | 41 ++++++++- ladon/arbiter/src/post.test.ts | 17 ++++ ladon/arbiter/src/post.ts | 2 +- ladon/review/action.yml | 2 +- ladon/setup/action.yml | 2 +- ladon/setup/dist/index.js | 2 +- .../setup/src/no-approval.integration.test.ts | 53 ++++++++--- ladon/setup/src/short-circuit.test.ts | 14 +++ ladon/setup/src/short-circuit.ts | 2 +- 14 files changed, 201 insertions(+), 47 deletions(-) diff --git a/internal/versioning/review-action.test.ts b/internal/versioning/review-action.test.ts index 1f13649..3b42503 100644 --- a/internal/versioning/review-action.test.ts +++ b/internal/versioning/review-action.test.ts @@ -68,8 +68,8 @@ describe('orchestrator approval boundary', () => { (s: ActionStep) => s.name === 'Validate inputs', ) - test('defaults compatibly and forwards the control to both actions', () => { - expect(manifest.inputs['auto-approve'].default).toBe('true') + test('defaults to no approval and forwards the control to both actions', () => { + expect(manifest.inputs['auto-approve'].default).toBe('false') for (const step of [setup, arbiter]) { expect(step.with['auto-approve']).toBe(`${'$'}{{ inputs.auto-approve }}`) } @@ -78,7 +78,14 @@ describe('orchestrator approval boundary', () => { ) }) - test.each(['false', '', 'False', 'off'])( + test.each([ + 'false', + '', + 'False', + 'off', + '${{ inputs.auto-approve }}', + ' true', + ])( 'auto-approve=%j prevents every stale reapproval API call even if the step runs', (value) => { const result = spawnSync( @@ -120,22 +127,27 @@ describe('orchestrator approval boundary', () => { expect(result.stdout).toContain('commit_id=exact-head') }) - test.each(['false', 'true', '', 'False', 'disabled'])( - 'validates auto-approve=%j before minting a write token', - (value) => { - const result = spawnSync('bash', ['-c', validate.run], { - encoding: 'utf8', - env: { - ...process.env, - AUTO_APPROVE: value, - CLIENT_ID: '123', - APP_ID: '', - }, - }) - expect(result.status).toBe(['false', 'true'].includes(value) ? 0 : 1) - expect(manifest.runs.steps[0]).toBe(validate) - }, - ) + test.each([ + 'false', + 'true', + '', + 'False', + 'disabled', + '${{ inputs.auto-approve }}', + ' true', + ])('validates auto-approve=%j before minting a write token', (value) => { + const result = spawnSync('bash', ['-c', validate.run], { + encoding: 'utf8', + env: { + ...process.env, + AUTO_APPROVE: value, + CLIENT_ID: '123', + APP_ID: '', + }, + }) + expect(result.status).toBe(['false', 'true'].includes(value) ? 0 : 1) + expect(manifest.runs.steps[0]).toBe(validate) + }) test('reviewer tool grants cannot submit approving reviews', () => { const reviewer = parse( @@ -206,3 +218,42 @@ describe('audited Ladon dependency pins', () => { }, ) }) + +describe('manifest omission defaults', () => { + test.each(['review', 'setup', 'arbiter'])( + '%s cannot enable approval by omission', + (name) => { + const action = parse( + readFileSync( + resolve(import.meta.dirname, `../../ladon/${name}/action.yml`), + 'utf8', + ), + ) + expect(action.inputs['auto-approve'].default).toBe('false') + }, + ) + test('omitted composite input cannot enter the stale reapproval write path', () => { + const action = parse( + readFileSync( + resolve(import.meta.dirname, '../../ladon/review/action.yml'), + 'utf8', + ), + ) + const step = action.runs.steps.find( + (s: ActionStep) => s.name === 'Re-approve dismissed stale approval', + ) + const result = spawnSync( + 'bash', + ['-c', `gh() { echo 'API CALLED'; }; ${step.run}`], + { + encoding: 'utf8', + env: { + ...process.env, + AUTO_APPROVE: action.inputs['auto-approve'].default, + }, + }, + ) + expect(result.status).toBe(1) + expect(result.stdout).not.toContain('API CALLED') + }) +}) diff --git a/ladon/arbiter/action.yml b/ladon/arbiter/action.yml index 7c6413e..30321a5 100644 --- a/ladon/arbiter/action.yml +++ b/ladon/arbiter/action.yml @@ -6,7 +6,7 @@ inputs: auto-approve: description: 'Whether Ladon may submit APPROVE (true or false). Set false for findings without approving reviews; blocking findings and escalations fail the action.' required: false - default: 'true' + default: 'false' anthropic-api-key: description: 'Anthropic API key' required: true diff --git a/ladon/arbiter/dist/index.js b/ladon/arbiter/dist/index.js index 21f5fe9..00009ac 100644 --- a/ladon/arbiter/dist/index.js +++ b/ladon/arbiter/dist/index.js @@ -47495,7 +47495,7 @@ function enforceDecisionGuards(decision, ctx) { // In findings-only mode the action result is a policy check, so enforce the // blocking rows of arbiter-decision.md before suppressing approval. An LLM // "approve" or "comment" must not turn a known blocker into a green check. - if (ctx.autoApprove === false) { + if (ctx.autoApprove !== true) { const findings = ctx.findings ?? []; const blockers = findings.filter((f) => f.severity === 'critical' || f.severity === 'high'); const mediums = findings.filter((f) => f.severity === 'medium'); @@ -47584,7 +47584,7 @@ function enforceDecisionGuards(decision, ctx) { summary: `${NO_AUTO_APPROVE_OVERRIDE_PREFIX}\n>\n> ${reason}\n\n${result.summary}`, }; } - if (ctx.autoApprove === false && result.outcome === 'approve') { + if (ctx.autoApprove !== true && result.outcome === 'approve') { const reason = 'Automatic approval is disabled (auto-approve=false). Ladon findings do not satisfy the required human review.'; overrides.push(reason); result = { @@ -47714,7 +47714,7 @@ async function postReview(params) { const { octokit, owner, repo, prNumber, headSha, body } = params; // Last boundary before the API: never approve and then dismiss. Auto-merge // can act on the first write, before a cleanup or a failing check completes. - const event = params.autoApprove === false && params.event === 'APPROVE' + const event = params.autoApprove !== true && params.event === 'APPROVE' ? 'COMMENT' : params.event; try { diff --git a/ladon/arbiter/src/enforce.test.ts b/ladon/arbiter/src/enforce.test.ts index c1a52c1..69ccada 100644 --- a/ladon/arbiter/src/enforce.test.ts +++ b/ladon/arbiter/src/enforce.test.ts @@ -1,5 +1,9 @@ import { describe, expect, test } from 'vitest' -import { enforceDecisionGuards } from './enforce.js' +import { enforceDecisionGuards as runDecisionGuards } from './enforce.js' + +// Existing compatibility-policy cases opt in explicitly at the test boundary. +const enforceDecisionGuards: typeof runDecisionGuards = (decision, context) => + runDecisionGuards(decision, { autoApprove: true, ...context }) const baseDecision = { outcome: 'approve' as const, @@ -332,3 +336,9 @@ test('high-risk flag alone does not force escalation in no-approval mode', () => }).decision.outcome, ).toBe('comment') }) + +test('an omitted guard capability defaults to an attributable non-approving decision', () => { + const result = runDecisionGuards(baseDecision, { authorTeamMatches: [] }) + expect(result.decision.outcome).toBe('comment') + expect(result.decision.summary).toContain('auto-approve=false') +}) diff --git a/ladon/arbiter/src/enforce.ts b/ladon/arbiter/src/enforce.ts index 99682e0..17eb222 100644 --- a/ladon/arbiter/src/enforce.ts +++ b/ladon/arbiter/src/enforce.ts @@ -34,7 +34,7 @@ export function enforceDecisionGuards( // In findings-only mode the action result is a policy check, so enforce the // blocking rows of arbiter-decision.md before suppressing approval. An LLM // "approve" or "comment" must not turn a known blocker into a green check. - if (ctx.autoApprove === false) { + if (ctx.autoApprove !== true) { const findings = ctx.findings ?? [] const blockers = findings.filter( (f) => f.severity === 'critical' || f.severity === 'high', @@ -148,7 +148,7 @@ export function enforceDecisionGuards( } } - if (ctx.autoApprove === false && result.outcome === 'approve') { + if (ctx.autoApprove !== true && result.outcome === 'approve') { const reason = 'Automatic approval is disabled (auto-approve=false). Ladon findings do not satisfy the required human review.' overrides.push(reason) diff --git a/ladon/arbiter/src/no-approval.integration.test.ts b/ladon/arbiter/src/no-approval.integration.test.ts index 9648f43..bab3698 100644 --- a/ladon/arbiter/src/no-approval.integration.test.ts +++ b/ladon/arbiter/src/no-approval.integration.test.ts @@ -1,11 +1,14 @@ +import { readFileSync } from 'node:fs' import { beforeEach, describe, expect, test, vi } from 'vitest' +import { parse } from 'yaml' import type { ArbiterDecision } from './anthropic.js' import type { Finding } from './findings.js' const fixture = vi.hoisted(() => { const inputs: Record = {} + const defaults: Record = {} const core = { - getInput: vi.fn((name: string) => inputs[name] ?? ''), + getInput: vi.fn((name: string) => inputs[name] ?? defaults[name] ?? ''), setOutput: vi.fn(), setFailed: vi.fn(), warning: vi.fn(), @@ -34,7 +37,7 @@ const fixture = vi.hoisted(() => { pull_request: { user: { login: 'author' }, base: { ref: 'main' } }, }, } - return { inputs, core, octokit, context, decide: vi.fn() } + return { defaults, inputs, core, octokit, context, decide: vi.fn() } }) vi.mock('@actions/core', () => fixture.core) @@ -84,6 +87,10 @@ async function run() { beforeEach(() => { vi.resetModules() vi.clearAllMocks() + // Model the runner's manifest-default injection for an omitted input. + fixture.defaults['auto-approve'] = parse( + readFileSync(new URL('../action.yml', import.meta.url), 'utf8'), + ).inputs['auto-approve'].default for (const key of Object.keys(fixture.inputs)) delete fixture.inputs[key] Object.assign(fixture.inputs, { 'auto-approve': 'false', @@ -258,7 +265,15 @@ describe('arbiter API and action result integration', () => { ) }) - test.each(['', 'False', '0', 'disabled', 'unresolved-expression'])( + test.each([ + '', + 'False', + '0', + 'disabled', + '${{ inputs.auto-approve }}', + ' false', + 'null', + ])( 'invalid auto-approve=%j fails before any review API call', async (input) => { fixture.inputs['auto-approve'] = input @@ -281,3 +296,23 @@ describe('arbiter API and action result integration', () => { expect(fixture.core.setFailed).toHaveBeenCalledWith('rejected') }) }) + +test('omitted input resolves the manifest default to false with zero approvals', async () => { + delete fixture.inputs['auto-approve'] + expect(fixture.defaults['auto-approve']).toBe('false') + await run() + expect( + fixture.octokit.rest.pulls.createReview, + ).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ event: 'COMMENT' }), + ) + expect(fixture.core.setFailed).not.toHaveBeenCalled() +}) + +test('missing raw input without runner defaults fails before any approval or reapproval', async () => { + delete fixture.inputs['auto-approve'] + delete fixture.defaults['auto-approve'] + await run() + expect(fixture.core.setFailed).toHaveBeenCalled() + expect(fixture.octokit.rest.pulls.createReview).not.toHaveBeenCalled() +}) diff --git a/ladon/arbiter/src/post.test.ts b/ladon/arbiter/src/post.test.ts index 7dc5f5e..4bf2b0c 100644 --- a/ladon/arbiter/src/post.test.ts +++ b/ladon/arbiter/src/post.test.ts @@ -65,6 +65,7 @@ describe('postReview', () => { prNumber: 1, headSha: 'h', event: 'APPROVE', + autoApprove: true, body: 'x', }) expect(createReview).toHaveBeenCalledTimes(2) @@ -341,3 +342,19 @@ describe('no-approval posting boundary', () => { }, ) }) + +test('an omitted final API capability never sends APPROVE, even before cleanup', async () => { + const createReview = vi.fn().mockResolvedValue({ data: { id: 1 } }) + await postReview({ + octokit: { rest: { pulls: { createReview } } } as never, + owner: 'o', + repo: 'r', + prNumber: 1, + headSha: 'h', + event: 'APPROVE', + body: 'findings', + }) + expect(createReview).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ event: 'COMMENT' }), + ) +}) diff --git a/ladon/arbiter/src/post.ts b/ladon/arbiter/src/post.ts index 0734855..7cc1426 100644 --- a/ladon/arbiter/src/post.ts +++ b/ladon/arbiter/src/post.ts @@ -76,7 +76,7 @@ export async function postReview(params: { // Last boundary before the API: never approve and then dismiss. Auto-merge // can act on the first write, before a cleanup or a failing check completes. const event = - params.autoApprove === false && params.event === 'APPROVE' + params.autoApprove !== true && params.event === 'APPROVE' ? 'COMMENT' : params.event try { diff --git a/ladon/review/action.yml b/ladon/review/action.yml index ed19be3..7acc589 100644 --- a/ladon/review/action.yml +++ b/ladon/review/action.yml @@ -6,7 +6,7 @@ inputs: auto-approve: description: 'Whether Ladon may submit APPROVE (true or false). Set false for findings without approving reviews; blocking findings and escalations fail the action.' required: false - default: 'true' + default: 'false' anthropic-api-key: description: 'Anthropic API key' required: true diff --git a/ladon/setup/action.yml b/ladon/setup/action.yml index fc930e1..97ea3b6 100644 --- a/ladon/setup/action.yml +++ b/ladon/setup/action.yml @@ -6,7 +6,7 @@ inputs: auto-approve: description: 'Whether Ladon may submit APPROVE (true or false). Set false for findings without approving reviews; blocking findings and escalations fail the action.' required: false - default: 'true' + default: 'false' github-token: description: 'GitHub App installation token' required: true diff --git a/ladon/setup/dist/index.js b/ladon/setup/dist/index.js index 44fecc8..6ebb07b 100644 --- a/ladon/setup/dist/index.js +++ b/ladon/setup/dist/index.js @@ -39816,7 +39816,7 @@ function evaluateShortCircuit(ctx) { return { shouldRun: false, skipReason: 'merge-conflicts' }; if (ctx.releaseStackBranches.includes(ctx.headRef)) return { shouldRun: false, skipReason: 'release-stack-branch' }; - if (ctx.eventAction === 'synchronize' && ctx.autoApprove !== false) { + if (ctx.eventAction === 'synchronize' && ctx.autoApprove === true) { if (ctx.isPureRebase) return { shouldRun: false, skipReason: 'pure-rebase' }; if (ctx.deltaFiles.length === 0 && diff --git a/ladon/setup/src/no-approval.integration.test.ts b/ladon/setup/src/no-approval.integration.test.ts index 06b75ce..a8c48a5 100644 --- a/ladon/setup/src/no-approval.integration.test.ts +++ b/ladon/setup/src/no-approval.integration.test.ts @@ -1,13 +1,15 @@ -import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterAll, beforeEach, describe, expect, test, vi } from 'vitest' +import { parse } from 'yaml' const fixture = vi.hoisted(() => { const inputs: Record = {} + const defaults: Record = {} const outputs: Record = {} const core = { - getInput: vi.fn((name: string) => inputs[name] ?? ''), + getInput: vi.fn((name: string) => inputs[name] ?? defaults[name] ?? ''), setOutput: vi.fn((name: string, value: string) => { outputs[name] = value }), @@ -48,7 +50,7 @@ const fixture = vi.hoisted(() => { }, }, } - return { inputs, outputs, core, octokit, context } + return { defaults, inputs, outputs, core, octokit, context } }) vi.mock('@actions/core', () => fixture.core) vi.mock('@actions/github', () => ({ @@ -79,6 +81,10 @@ async function run() { beforeEach(() => { vi.resetModules() vi.clearAllMocks() + // Model the runner's manifest-default injection for an omitted input. + fixture.defaults['auto-approve'] = parse( + readFileSync(new URL('../action.yml', import.meta.url), 'utf8'), + ).inputs['auto-approve'].default vi.stubEnv('GITHUB_WORKSPACE', directory) for (const object of [fixture.inputs, fixture.outputs]) { for (const key of Object.keys(object)) delete object[key] @@ -215,14 +221,35 @@ describe('setup stale-approval integration', () => { expect(fixture.outputs.reapprove).toBeUndefined() }) - test.each(['', 'FALSE', 'off'])( - 'invalid input %j fails closed', - async (input) => { - fixture.inputs['auto-approve'] = input - await run() - expect(fixture.core.setFailed).toHaveBeenCalled() - expect(fixture.outputs.reapprove).toBeUndefined() - expect(fixture.octokit.rest.pulls.createReview).not.toHaveBeenCalled() - }, - ) + test.each([ + '', + 'FALSE', + 'off', + '${{ inputs.auto-approve }}', + ' false', + 'null', + ])('invalid input %j fails closed', async (input) => { + fixture.inputs['auto-approve'] = input + await run() + expect(fixture.core.setFailed).toHaveBeenCalled() + expect(fixture.outputs.reapprove).toBeUndefined() + expect(fixture.octokit.rest.pulls.createReview).not.toHaveBeenCalled() + }) +}) + +test('omitted input resolves the manifest default to false with zero approvals', async () => { + delete fixture.inputs['auto-approve'] + expect(fixture.defaults['auto-approve']).toBe('false') + await run() + expect(fixture.outputs['should-run']).toBe('true') + expect(fixture.outputs.reapprove).toBeUndefined() + expect(fixture.octokit.rest.pulls.createReview).not.toHaveBeenCalled() +}) + +test('missing raw input without runner defaults fails before any approval or reapproval', async () => { + delete fixture.inputs['auto-approve'] + delete fixture.defaults['auto-approve'] + await run() + expect(fixture.core.setFailed).toHaveBeenCalled() + expect(fixture.octokit.rest.pulls.createReview).not.toHaveBeenCalled() }) diff --git a/ladon/setup/src/short-circuit.test.ts b/ladon/setup/src/short-circuit.test.ts index 20f2465..806e2c8 100644 --- a/ladon/setup/src/short-circuit.test.ts +++ b/ladon/setup/src/short-circuit.test.ts @@ -8,6 +8,8 @@ function ctx( overrides: Partial = {}, ): ShortCircuitContext { return { + // Legacy shortcut tests explicitly opt in; omission must review fully. + autoApprove: true, prState: 'open', isDraft: false, hasForceReviewLabel: false, @@ -192,3 +194,15 @@ describe('no-approval mode', () => { ).toEqual({ shouldRun: false, skipReason: 'bot-author' }) }) }) + +test('omitting the shortcut capability cannot skip a stale dismissed approval', () => { + expect( + evaluateShortCircuit( + ctx({ + autoApprove: undefined, + eventAction: 'synchronize', + isPureRebase: true, + }), + ), + ).toEqual({ shouldRun: true, skipReason: null }) +}) diff --git a/ladon/setup/src/short-circuit.ts b/ladon/setup/src/short-circuit.ts index 439055b..36833d6 100644 --- a/ladon/setup/src/short-circuit.ts +++ b/ladon/setup/src/short-circuit.ts @@ -46,7 +46,7 @@ export function evaluateShortCircuit( if (!ctx.mergeable) return { shouldRun: false, skipReason: 'merge-conflicts' } if (ctx.releaseStackBranches.includes(ctx.headRef)) return { shouldRun: false, skipReason: 'release-stack-branch' } - if (ctx.eventAction === 'synchronize' && ctx.autoApprove !== false) { + if (ctx.eventAction === 'synchronize' && ctx.autoApprove === true) { if (ctx.isPureRebase) return { shouldRun: false, skipReason: 'pure-rebase' } if ( ctx.deltaFiles.length === 0 && From 3d180c9b365c201d4cd4cfd93d0c2a7e8790da70 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 14 Sep 2026 18:16:40 +0000 Subject: [PATCH 07/11] fix(ladon): pin actions that deny approval by default --- ladon/review/action.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ladon/review/action.yml b/ladon/review/action.yml index 7acc589..946fa68 100644 --- a/ladon/review/action.yml +++ b/ladon/review/action.yml @@ -97,7 +97,7 @@ runs: # Update these pins explicitly when promoting a reviewed sub-action change. - name: Setup id: setup - uses: adcontextprotocol/actions/ladon/setup@292a0da93b25c3e9ecfcf5a05763a4206c472c3a + uses: adcontextprotocol/actions/ladon/setup@02db55f54f39f93c683c2031887b735aea0ecba2 with: github-token: ${{ steps.app-token.outputs.token }} ladon-bot-login: ${{ steps.app-token.outputs.app-slug }}[bot] @@ -162,7 +162,7 @@ runs: - name: Arbiter if: steps.setup.outputs.should-run == 'true' id: arbiter - uses: adcontextprotocol/actions/ladon/arbiter@292a0da93b25c3e9ecfcf5a05763a4206c472c3a + uses: adcontextprotocol/actions/ladon/arbiter@02db55f54f39f93c683c2031887b735aea0ecba2 with: anthropic-api-key: ${{ inputs.anthropic-api-key }} github-token: ${{ steps.app-token.outputs.token }} From bc714566a9da2a77d602e69420d9d46700435a00 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 14 Sep 2026 18:19:49 +0000 Subject: [PATCH 08/11] fix(ladon): adopt default-deny self-review and audit every consumer --- .github/workflows/ai-review.yml | 10 +- internal/versioning/ladon-consumers.test.ts | 158 +++++++ ladon/HUMAN-APPROVAL-ADOPTION.md | 450 ++++++++++---------- ladon/INSTALL.md | 11 +- ladon/README.md | 6 +- ladon/consumer-policy.json | 10 + scripts/audit-ladon-consumers.cjs | 243 +++++++++++ 7 files changed, 648 insertions(+), 240 deletions(-) create mode 100644 internal/versioning/ladon-consumers.test.ts create mode 100644 ladon/consumer-policy.json create mode 100644 scripts/audit-ladon-consumers.cjs diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index fd1663d..931d54a 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -1,13 +1,12 @@ name: AI PR Review (Ladon) # Ladon reviews every non-draft, non-dependabot PR. The heavy lifting lives in -# the action tree under ladon/ (setup -> reviewer -> arbiter), orchestrated by -# ladon/review. This workflow handles the pull_request_target security posture +# the immutable reviewed Ladon action (setup -> reviewer -> arbiter). This workflow handles the pull_request_target security posture # (trusted base-SHA checkout only; the PR head is never fetched, checked out, or # executed - setup reads head content via the GitHub API), the # review-workflow-modification gate, and invoking the composite. Reviews post as -# the AAO Secretariat App so they count toward the "1 review required" -# branch-protection check. +# the AAO Secretariat App as automated findings. Human approval is separately +# required. HUMAN MERGE HOLD: see ladon/HUMAN-APPROVAL-ADOPTION.md. # # pull_request_target (not pull_request) is required on a public repo so that PRs # from forks can access the App token and Anthropic key. @@ -88,8 +87,9 @@ jobs: - name: Run Ladon if: steps.workflow-mod.outputs.modified != 'true' - uses: ./ladon/review + uses: adcontextprotocol/actions/ladon/review@3d180c9b365c201d4cd4cfd93d0c2a7e8790da70 with: + auto-approve: 'false' anthropic-api-key: ${{ secrets.ANTHROPIC_API_KEY }} app-id: ${{ secrets.SECRETARIAT_APP_ID }} app-private-key: ${{ secrets.SECRETARIAT_APP_PRIVATE_KEY }} diff --git a/internal/versioning/ladon-consumers.test.ts b/internal/versioning/ladon-consumers.test.ts new file mode 100644 index 0000000..b5451a1 --- /dev/null +++ b/internal/versioning/ladon-consumers.test.ts @@ -0,0 +1,158 @@ +import { readFileSync } from 'node:fs' +import { createRequire } from 'node:module' +import { resolve } from 'node:path' +import { describe, expect, test } from 'vitest' + +const { validateInventory, invocations } = createRequire(import.meta.url)( + '../../scripts/audit-ladon-consumers.cjs', +) +const policy = JSON.parse( + readFileSync( + resolve(import.meta.dirname, '../../ladon/consumer-policy.json'), + 'utf8', + ), +) +const workflow = ( + uses = `adcontextprotocol/actions/ladon/review@${policy.review}`, + input: unknown = 'false', +) => + JSON.stringify({ + jobs: { review: { steps: [{ uses, with: { 'auto-approve': input } }] } }, + }) +const report = () => ({ + complete: true, + search: { incomplete_results: false, total_count: 0, items: [] }, + repositories: policy.repositories.map((name: string) => ({ + name, + head: 'a'.repeat(40), + workflows: [ + { path: '.github/workflows/ai-review.yml', content: workflow() }, + ], + })), +}) + +describe('organization Ladon release inventory is fail closed', () => { + test('all five intended consumers are explicitly inventoried with an immutable review pin', () => { + expect(policy.repositories).toEqual([ + 'adcontextprotocol/actions', + 'adcontextprotocol/adcp', + 'adcontextprotocol/adcp-client', + 'adcontextprotocol/adcp-go', + 'adcontextprotocol/adcp-client-python', + ]) + expect(policy.review).toMatch(/^[a-f0-9]{40}$/) + expect(validateInventory(report())).toEqual([]) + }) + test('a sixth consumer fails even if pinned and disabled', () => { + const value = report() + value.repositories.push({ + name: 'adcontextprotocol/new-sdk', + head: 'b'.repeat(40), + workflows: [ + { path: '.github/workflows/review.yml', content: workflow() }, + ], + }) + expect(validateInventory(value)).toContain( + 'Uninventoried Ladon consumer: adcontextprotocol/new-sdk', + ) + }) + test.each([ + undefined, + null, + '', + true, + false, + 'true', + 'False', + '${{ inputs.auto-approve }}', + ])('missing or nonliteral disabled input %j blocks promotion', (input) => { + const value = report() + value.repositories[0].workflows[0].content = JSON.stringify({ + jobs: { + r: { + steps: [ + { + uses: `adcontextprotocol/actions/ladon/review@${policy.review}`, + with: input === undefined ? {} : { 'auto-approve': input }, + }, + ], + }, + }, + }) + expect(validateInventory(value).join('\n')).toContain( + "auto-approve must be literal 'false'", + ) + }) + test.each([ + './ladon/review', + 'adcontextprotocol/actions/ladon/review@ladon/review/v1', + `adcontextprotocol/actions/ladon/review@${'f'.repeat(40)}`, + ])('local/floating/wrong pin %s blocks promotion', (pin) => { + const value = report() + value.repositories[0].workflows[0].content = workflow(pin) + expect(validateInventory(value).join('\n')).toContain( + 'missing/wrong immutable pin', + ) + }) + test('missing consumer and additional invocation fail closed', () => { + const value = report() + value.repositories.pop() + value.repositories[0].workflows.push({ + path: '.github/workflows/second.yml', + content: workflow(), + }) + expect(validateInventory(value).length).toBe(2) + }) + test('partial search and incomplete enumeration cannot certify rollout', () => { + const value = report() + value.complete = false + value.search.incomplete_results = true + value.search.total_count = 1 + expect(validateInventory(value).length).toBe(3) + }) + test('a search hit not reconciled to an exact-head workflow blocks rollout', () => { + const value = report() + const search = { + incomplete_results: false, + total_count: 1, + items: [ + { + repository: { full_name: 'adcontextprotocol/unknown' }, + path: '.github/workflows/review.yml', + }, + ], + } + expect(validateInventory({ ...value, search }).join('\n')).toContain( + 'Unreconciled code-search result', + ) + }) + test('comments and release tag strings are not invocation sites', () => { + expect( + invocations({ + jobs: { publish: { steps: [{ run: 'echo ladon/review/v1' }] } }, + }), + ).toEqual([]) + }) + test('malformed workflow parsing throws rather than blessing a partial inventory', () => { + const value = report() + value.repositories[0].workflows[0].content = 'jobs: [broken' + expect(() => validateInventory(value)).toThrow() + }) + test('actions own trusted workflow is pinned and disabled with the original human modification gate', () => { + const value = report() + value.repositories[0].workflows[0].content = readFileSync( + resolve(import.meta.dirname, '../../.github/workflows/ai-review.yml'), + 'utf8', + ) + expect(validateInventory(value)).toEqual([]) + expect(value.repositories[0].workflows[0].content).toContain( + "if: steps.workflow-mod.outputs.modified != 'true'", + ) + expect(value.repositories[0].workflows[0].content).toContain( + 'ladon/*|.github/workflows/ai-review.yml|LADON.md)', + ) + expect(value.repositories[0].workflows[0].content).toContain( + "event: 'COMMENT'", + ) + }) +}) diff --git a/ladon/HUMAN-APPROVAL-ADOPTION.md b/ladon/HUMAN-APPROVAL-ADOPTION.md index 8d7c43e..d03e4e9 100644 --- a/ladon/HUMAN-APPROVAL-ADOPTION.md +++ b/ladon/HUMAN-APPROVAL-ADOPTION.md @@ -1,92 +1,65 @@ # AdCP human approval adoption -This change provides an opt-in no-approval boundary. **It does not by itself fix -branch protection.** Both consuming workflows must adopt it, existing approvals -and in-flight old runs must be accounted for, and maintainers must audit the -rulesets. Keep the actions PR Draft and auto-merge off during coordination. - -## Incident and audited revisions - -On 2026-09-14, [adcp-client #2911](https://github.com/adcontextprotocol/adcp-client/pull/2911) -merged with only an AAO Secretariat App approval at head -`a6834d67870253674178d099447af36dd6640b11`; `bmilekic` remained requested and had -not reviewed. The PR author enabled auto-merge. REST evidence records review -5200764177 at `17:23:07Z` and merge `8ba12c2ace85a88533ce1d56efd35badea51a97c` -at `17:23:15Z`. Green source CI and an independent agent audit were not human -approval. The incident report records no package release; -[Version Packages #2912](https://github.com/adcontextprotocol/adcp-client/pull/2912) -was confirmed open, Draft, and without auto-merge during this audit. Its -quarantine is outside this change. - -Complete consuming workflows were read at these current-main snapshots: - -| Repository | Main commit | Tree | -| ----------- | ------------------------------------------ | ------------------------------------------ | -| actions | `a64a17ba369122d6b3401f614a31df7b8607f043` | `0029081e46f388904c186c3402a0a4eb9dd523cc` | -| adcp | `36a86d6c4ae5c42e5e2e783d95bdb97b5cb6f614` | `c285d352a02be51e4a787d6208d52230dea81330` | -| adcp-client | `8ba12c2ace85a88533ce1d56efd35badea51a97c` | `1771a8ef743b4ddcd89c836cee39563096dac82e` | - -Sources: [adcp workflow](https://github.com/adcontextprotocol/adcp/blob/36a86d6c4ae5c42e5e2e783d95bdb97b5cb6f614/.github/workflows/ai-review.yml), -[adcp-client workflow](https://github.com/adcontextprotocol/adcp-client/blob/8ba12c2ace85a88533ce1d56efd35badea51a97c/.github/workflows/ai-review.yml). -Both use `pull_request_target`, a trusted base checkout, `code_review`, the -review-workflow-modification gate, and `@ladon/review/v1`. The client workflow -explicitly says App reviews count toward “1 review required.” Neither supplies -an approval control at the audited revision. - -At audit time, review/setup/arbiter floating v1 tags resolved to -`ab7c8394702dad2244b48143c78b6651cd5215ed`; reviewer v1 resolved to the actions -base commit above. No tag was moved by this work. - -## Follow-up incident evidence and global hold (2026-09-14 UTC) - -The original snapshots above are historical. Consumer adoption was refreshed to -adcp `1467e46117e8d329f4116e548e9290be75d8e3b3` (tree -`023fac83c8812e6148519ba8a4cfe2a49fbb3070`) and client -`55829081dae2673f73615af2a4f6f029cddff813` (tree -`75aa2d649b3234643ec4619ea7b65a8b86d44320`). Existing open PR evidence, including -adcp #7450/#7462, remains on its recorded older base until independently -refreshed; this change does not recompute their merge trees. - -[adcp #7521](https://github.com/adcontextprotocol/adcp/pull/7521) demonstrates a -**second, distinct bypass**. Base was -`36a86d6c4ae5c42e5e2e783d95bdb97b5cb6f614`; head was -`e680c97e05e69cf3d7b6bf60aed07c42657c420c`. Its only review, Secretariat -`5200346889`, was COMMENTED at `16:38:41Z` with an explicit Ladon human/CODEOWNERS -escalation for modified -`static/schemas/source/compliance/comply-test-controller-{request,response}.json` -and `REVIEW_REQUIRED`. There were **zero APPROVED reviews**. Author `bokelley` -merged it himself at `18:02:02Z` as -`1467e46117e8d329f4116e548e9290be75d8e3b3`; final `auto_merge` was null. -An escalation comment or optional check cannot stop an author with effective -merge permission. The precise bypass rule still requires administrator audit. - -[adcp-client #2913](https://github.com/adcontextprotocol/adcp-client/pull/2913) -had only bot approval at exact head -`bfacfeb66178d161254830faef48f36a5a6a629e`: Secretariat review `5201034616` -APPROVED at `17:51:07Z`, while `bmilekic` remained requested without reviewing. -After the coordinator disabled auto-merge and placed it Draft, author `bokelley` -re-enabled auto-merge at `17:57:24Z`. It merged at `17:59:28Z` as -`55829081dae2673f73615af2a4f6f029cddff813` (base -`8ba12c2ace85a88533ce1d56efd35badea51a97c`). Secretariat review `5201137108` -APPROVED that same head at `18:01:40Z`, **after merge**. The later approval -cannot supply human evidence or explain the earlier merge; it confirms older -approval-capable runs can continue writing. Draft/auto-off comments are -advisory against an author/admin and are not a protection boundary. - -Generated #2912 advanced to `1be6b25e8083601f8bf08f1b701435181f33215f` on -base `55829081dae2673f73615af2a4f6f029cddff813`, and was still Draft/auto-off. -Coordinator evidence reports npm dist-tags unchanged at rc.36 / latest 13.0.4, -with no new package published. **Maintain a global release/merge hold** until -both human-reviewed immutable consumer pins land and an authorized human -configures and validates an actually required, trusted exact-head human gate. -PR #29 alone cannot prevent manual author/admin bypass or older runs. - -Consumer runtime pins stay at reviewed orchestrator -`85284cbc297875aa04b2d5afcf7ae833b29f329c`, nested setup/arbiter -`292a0da93b25c3e9ecfcf5a05763a4206c472c3a`, and reviewer -`a64a17ba369122d6b3401f614a31df7b8607f043`. Documentation-only updates to this -PR do not require moving those runtime pins. The consumer configuration tests -reject missing approval inputs; the shared compatible default remains true. +**Branch protection is not fixed by this PR. Maintain the global release/merge +hold.** Reviews default to findings without approval; the shared action still +cannot prevent author/admin bypass, other Apps, or old approval-capable runs. +Keep #29 and all four external adoption PRs Draft/auto-off. Requesting a human +reviewer or obtaining an agent audit does not satisfy human approval. + +## Complete intended Ladon consumer inventory + +The original two-repository inventory was incomplete. All **five** consumers +are in scope; none has explicit human risk acceptance for an exclusion. +Snapshots below record the unsafe pre-adoption state, not an enforcement claim. + +| Repository | Exact main snapshot | Existing invocation | +| ------------------ | ------------------------------------------ | ----------------------------------------------------------------- | +| actions | `a64a17ba369122d6b3401f614a31df7b8607f043` | local `./ladon/review`, approving default | +| adcp | `1467e46117e8d329f4116e548e9290be75d8e3b3` | floating `@ladon/review/v1`, missing input | +| adcp-client | `55829081dae2673f73615af2a4f6f029cddff813` | floating `@ladon/review/v1`, missing input, claims App counts | +| adcp-go | `2f99fb0e8ede3639cc367577decfc5a544af297a` | floating `@ladon/review/v1`, missing input, claims App counts | +| adcp-client-python | `09382bb47e8c03f340c6cf4370f9f65be09dce62` | old pin `48c0dfef96594f74f9606d68597c0f7062a5a337`, missing input | + +All invocation sites are `.github/workflows/ai-review.yml`. Exact code search +`org:adcontextprotocol "ladon/review" path:.github/workflows` returned six +files: these five invocations plus actions' `merge-publish.yml` release metadata +(a string reference, not a review invocation). The audit additionally enumerates +all visible active organization repositories and reads their workflow blobs at +exact default-branch commits because search indexing can lag. A human must +confirm the credential can see the entire intended organization. + +`ladon/consumer-policy.json` is the explicit inventory. The deterministic tests +reject a sixth consumer, missing expected consumers, added invocation sites, +wrong/local/floating pins, missing/nonliteral/true inputs, malformed YAML, +partial search results and incomplete enumeration. No approving consumer is +accepted. Explicit true compatibility is tested solely as a supported opt-in; +adding an approving consumer would require separate human policy acceptance, +recorded inventory and a reviewed change to this enforcement contract. + +## Reviewed immutable implementation + +- Orchestrator and approval-boundary regression suite: + `3d180c9b365c201d4cd4cfd93d0c2a7e8790da70`. +- Nested setup/arbiter manifests, source and executable bundles: + `02db55f54f39f93c683c2031887b735aea0ecba2`. +- Nested reviewer: `a64a17ba369122d6b3401f614a31df7b8607f043`. + +All five proposed workflows pin that orchestrator and set `auto-approve: 'false'` +explicitly. Actions' own workflow changes as part of #29, retains the trusted +base checkout and original `ladon/*`/workflow/LADON.md human modification gate, +and cannot approve itself. All four external PRs preserve their existing human +modification gates. Python removes its legacy post-review retirement invocation, +which trusted a bot approval; the reviewed arbiter handles superseded findings +after complete review. The old retirement script remains protected by Python's +existing modification gate. Do not revive it as a cleanup-based approval guard. + +Unlike superseded head `85284cbc297875aa04b2d5afcf7ae833b29f329c`, this artifact +**defaults false in all three manifests**. Direct helper omissions also deny +approval/shortcuts. Raw empty/malformed/nonliteral inputs fail before reviews; +composite validation precedes App token minting. Tests cover runner-injected +omission defaults and missing raw input, with zero APPROVE/reapprove API calls. +The new default is a behavior change: legacy approval requires explicit true. +No existing floating tag or published action has been changed by this PR. ## Approval paths and the new boundary @@ -111,7 +84,9 @@ reject missing approval inputs; the shared compatible default remains true. never produce approval. Set `auto-approve: 'false'` in the **trusted consuming workflow**, not in PR -prose or LADON.md. The manifest default stays `'true'` for existing consumers. +prose or LADON.md. The review/setup/arbiter manifest defaults are now `'false'`: omission +cannot enable approval. Only explicit `'true'` opts into compatibility behavior, +and the intended organization inventory rejects that mode. Explicit empty, malformed, or unsupported values fail before posting. The same input is available on setup and arbiter for direct users; they must apply it consistently and remove/guard any approval code in their own orchestrator. @@ -162,141 +137,164 @@ The legacy `hasHumanApproval` helper is a supplemental gated-path check, not an exact-head, authorized-human gate: it does not verify latest per-user review, commit, author exclusion, or reviewer authority. Do not use it as that gate. -## Coordinated landing order - -1. **Hold merges during migration.** Human maintainers keep affected PRs Draft - or auto-merge disabled, identify/cancel/drain old approval-capable runs, and - inspect active Ladon approvals. Dismiss obsolete bot approvals while the - hold is in place. Never depend on racing a cleanup against auto-merge. -2. Obtain exact-head CI and independent security review for the actions Draft - PR, then explicit human review. Record base/head/tree, immutable sub-action - pins, tests, and review evidence in the PR. No publishing or tag movement - is authorized by this preparation. -3. Open **separate human-reviewed follow-up PRs in adcp and adcp-client**. In - each `.github/workflows/ai-review.yml`, pin `Run Ladon` to the human-reviewed - actions PR head SHA (replace `REVIEWED_ACTIONS_SHA` below), and add the input: - - ```yaml - - name: Run Ladon - if: steps.workflow-mod.outputs.modified != 'true' - uses: adcontextprotocol/actions/ladon/review@REVIEWED_ACTIONS_SHA - with: - auto-approve: "false" - # Retain each repository's existing credentials, model, and bot list. - ``` - - Remove the client's claim that Ladon satisfies “1 review required.” State - in both workflows that Ladon provides automated findings and human approval - is separate. Preserve `pull_request_target`, base-SHA checkout, `code_review`, - permissions and the workflow-modification gate. Preserve each bot list - (`aao-release-bot[bot]` in adcp; `aao-ipr-bot[bot]` in adcp-client). - Do not relax the gate to test the new workflow against its own PR head. - Pure workflow edits are path-filtered; mixed PRs get only the base - workflow's COMMENT and skip Ladon. Neither follow-up can self-approve. - If filtered required checks stay pending, a maintainer must resolve that - through the audited human procedure; do not manufacture a green result. - -4. **Land both consumer PRs with real human approval before moving - `ladon/review/v1`.** Pinning the reviewed implementation makes the new mode - effective without waiting for a tag. Confirm both default-branch workflows - contain the pin/input and inspect runs at those revisions. Keep old runs - and pre-existing approvals under the migration hold until accounted for. -5. Complete the ruleset audit below. Only after both consumers have landed and - verification is recorded may maintainers authorize merging/publishing the - actions change. **Merging this PR to actions/main automatically invokes - `merge-publish.yml`**, which advances affected patch and floating major tags; - `manual-version-publish.yml` can also move them. Thus merging the actions PR - first would violate the required order even without a manual tag command. - No tag, merge, deployment, or publication is performed in this task. -6. Keep the consumer SHA pins for reproducibility, or human-review a later - switch to the promoted `@ladon/review/v1`, retaining `auto-approve: 'false'`. - A later workflow change is subject to the same human-only modification gate. - -## Required manual ruleset audit - -Read-only ruleset inspection was available. Both repositories inherit active -organization ruleset `8519441` (one approval, CODEOWNERS required, admin/role -and integration bypasses). Their active repository rulesets `15545291` (adcp) -and `15545837` (client) additionally dismiss stale approvals and require -last-push approval. These are **approval-count rules**, not proof of a human. -The visible required-check lists include source CI and IPR, but neither -`code_review` nor a separate human-review check. The classic branch-protection -API returned **403** for both; therefore this is not a complete effective-rule -audit. No settings were changed. - -An authorized maintainer must inspect repository **and inherited organization** -rulesets, classic protection, CODEOWNERS coverage, App permissions and all -bypass actors. Confirm which exact check contexts and expected App identities -are required, including skipped/path-filtered cases. Record the actual rules -and verification evidence; requesting a reviewer is not approval. - -If a human-only approval requirement is not already independently enforced, -add a trusted required check (for example `Human review / exact head`) with -these properties before lifting the hold: - -- Read live PR head and paginated reviews using trusted base code. Count only - the latest effective non-dismissed approval from an authorized maintainer or - required owner at that exact head; exclude the PR author, all Apps/bots and - explicitly identified automation accounts, including those typed as User. -- Verify current reviewer authority and CODEOWNERS/required-team policy; fail - closed on unknown identity, permissions, incomplete data, or API failure. -- Recompute on synchronize, ready-for-review, review submission/dismissal and - other policy-affecting events. Tie status to the live head, reject stale - workers, and reset on head/approval changes. Review-only events must not - execute PR-head code or expose write credentials to it. -- Publish a uniquely named required status/check from a trusted producer, - configure the expected App where supported, and cover workflow-modifying, - bot-authored, fork, rebase, and merge-queue cases. Skips must never mean - “human approved.” Audit bypasses and concurrent review dismissal behavior. -- Verify on controlled PRs: bot-only approval stays blocked, requested-but-no- - review stays blocked, dismissed/old-head/self approval stays blocked, an - authorized exact-head human approval lifts only the human gate, and blocking - Ladon findings still prevent merging under the repository's policy. - -[GitHub documents required reviews, bypass behavior, and expected check sources](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches). -This separate gate must itself be installed through human-reviewed workflow -and ruleset changes. The shared action cannot supply repository-wide human -identity enforcement with its current workflow permissions and event coverage. -Do not report branch protection fixed until consumer adoption and the -applicable tag/ruleset changes are actually complete. - -### Administrator action and controlled validation after #7521/#2913 - -An explicitly authorized administrator must export the effective rules for -main from repository Settings → Rules → Rulesets, inherited organization -rules, classic protection, CODEOWNERS and every role/App/direct-push bypass. -Determine the exact permission that allowed #7521's author to merge without any -approval. Install the separately human-reviewed trusted producer and require -`Human review / exact head` from its specific App identity; require blocking -Ladon results under an explicit workflow-modification/high-risk exception -policy as well. The new consumer `Ladon approval policy` CI job is only a -configuration/regression check, not this human gate. - -The human gate must exclude the PR author, verify **current** collaborator and -CODEOWNER/team authority, and reject bots/Apps and known automation accounts -masquerading as users. Make it non-bypassable by ordinary authors, including -the author role implicated by #7521. Remove broad role/App bypasses or confine -any emergency override to an independently controlled, logged procedure; -prevent direct pushes and alternative merge entrypoints from bypassing it. -No settings change is authorized by this preparation or by green CI. - -Recompute and invalidate on head, review/dismissal, base/CODEOWNERS and -membership/permission changes. Serialize workers and re-read live head, -review state and authority before publishing; use native stale-dismissal and -last-push rules too. Status webhooks are asynchronous: do not assert atomic -review-dismissal/merge safety without evidence. Retain the operational hold -and an authorized trusted merge procedure if that race is not closed. - -Start controlled validation on a non-production branch with matching effective -protection and no deployment/release hooks. Actual merge attempts require -separate explicit administrator authority. Prove rejection under ordinary -author credentials for no review, bot-only approval, COMMENT-only escalation -(#7521), requested-but-absent review, self/old-head/dismissed approval, lost -collaborator/CODEOWNER authority, missing/skipped/failed status, API failure, -stale worker, fork, workflow modification, rebase and merge queue. Exercise -concurrent push, dismissal and auto-merge. A current-head authorized non-author -human approval satisfies only the human gate; blocking Ladon findings must -remain blocking. Then verify actual main's effective rules and producer App; -a sandbox-only result does not prove main enforcement. Record rule exports, -exact heads/statuses/reviewer authority, denied merge attempts and a named -administrator's acceptance before lifting the global hold or promoting tags. +## Three incidents, two distinct bypasses (2026-09-14 UTC) + +1. [adcp-client #2911](https://github.com/adcontextprotocol/adcp-client/pull/2911) + had only `aao-secretariat[bot]` APPROVED at exact head + `a6834d67870253674178d099447af36dd6640b11`. Requested `bmilekic` never reviewed. + REST review `5200764177` was submitted at `17:23:07Z`; author-enabled + auto-merge completed at `17:23:15Z` as + `8ba12c2ace85a88533ce1d56efd35badea51a97c`. No package release was recorded. +2. [adcp #7521](https://github.com/adcontextprotocol/adcp/pull/7521) + had **zero APPROVED reviews**. Its only review, `5200346889`, was + `aao-secretariat[bot]` COMMENTED at `16:38:41Z` on exact head + `e680c97e05e69cf3d7b6bf60aed07c42657c420c`, explicitly escalating changes to + `static/schemas/source/compliance/comply-test-controller-{request,response}.json` + for human/CODEOWNERS approval with `REVIEW_REQUIRED`. Base was + `36a86d6c4ae5c42e5e2e783d95bdb97b5cb6f614`. Author `bokelley` nevertheless + merged it himself at `18:02:02Z` as + `1467e46117e8d329f4116e548e9290be75d8e3b3`; final `auto_merge` was null. + This demonstrates author self-merge despite an explicit human escalation; + disabling bot APPROVE addresses only the first incident's approval path. + It does not prove which effective bypass permission enabled the second. + +3. [adcp-client #2913](https://github.com/adcontextprotocol/adcp-client/pull/2913) + had only bot approval at exact head + `bfacfeb66178d161254830faef48f36a5a6a629e`: Secretariat review `5201034616` + APPROVED at `17:51:07Z`; `bmilekic` remained requested without reviewing. + After the coordinator disabled auto-merge and put the PR in Draft, author + `bokelley` re-enabled auto-merge at `17:57:24Z` and it merged at `17:59:28Z` + as `55829081dae2673f73615af2a4f6f029cddff813` (base + `8ba12c2ace85a88533ce1d56efd35badea51a97c`). Bot review `5201137108` + APPROVED the same head at `18:01:40Z`, **after merge**; it cannot be human + evidence or a cause of that merge. Old approval-capable runs can still write + approvals, and Draft/auto-off comments are advisory against an author/admin. + +Initial inspection found inherited organization ruleset `8519441` and repository +rulesets `15545291` (adcp) / `15545837` (client). Approval-count/CODEOWNERS rules +and stale/last-push rules were visible, with role/admin/integration bypasses. +Visible required checks did not include `code_review` or a separate human gate. +Classic protection returned 403, so this was **not a complete effective audit**. +An escalation comment or an optional failed check does not enforce a merge hold. + +## Exact organization audit before and after publication + +Run from a trusted checkout with read-only GitHub access after the four external +pins land and the actions candidate has exact CI, independent audit and human +approval: + +```sh +npm ci --ignore-scripts +node scripts/audit-ladon-consumers.cjs --actions-head EXACT_REVIEWED_ACTIONS_PR_HEAD > candidate-inventory.json +``` + +The option accepts only an immutable actions SHA and replaces **only** actions' +workflow snapshot; all four external consumers must already be safe on their +live default branches. The report explicitly says it is a pre-publication +candidate, **not live enforcement**. This avoids pretending actions/main is +already safe before #29 lands. Any unsafe/unknown site, partial API data or +head movement returns nonzero and keeps the hold. Do not use a PR head override +for an external consumer to bypass landing order. + +After separately authorized publication, repeat without the option to audit all +five live defaults: + +```sh +node scripts/audit-ladon-consumers.cjs > live-inventory.json +``` + +Neither command modifies GitHub, tags or rulesets, and neither replaces the +administrator's human-gate audit. Current live runs are expected to fail while +unsafe old workflows remain; do not turn that into an unconditional green CI. + +## Coordinated dependency order and quarantine + +1. Maintain the operational human merge hold. Inventory active bot approvals and + all older runs (including reruns, queued jobs, other review entrypoints and + App/machine users). Authorized maintainers must cancel/drain them and dismiss + obsolete approvals under the hold. COMMENT never revokes an old approval; + this pin cannot stop an in-flight older action. Do not race cleanup against + auto-merge. No such cancellation/dismissal is performed by this PR. +2. Obtain exact-head CI and independent audit, then actual human review of + **all four** separate external consumer PRs. Land all four external immutable pins through the audited + human procedure. External consumers may land in any order; all four precede actions + promotion. Do not loosen the workflow-modification gate for testing. +3. Complete the administrator audit/controlled validation below before lifting + the hold or merging actions #29. Merging actions/main invokes automatic tag + publication. **Do not merge #29 or move `ladon/review/v1` until all four + human-reviewed external consumer pins have landed and the effective human gate is + verified.** Keep consumer SHA pins after promotion. +4. Keep [Version Packages #2912](https://github.com/adcontextprotocol/adcp-client/pull/2912) + quarantined Draft/auto-off. Its generated head advanced to + `1be6b25e8083601f8bf08f1b701435181f33215f` on base + `55829081dae2673f73615af2a4f6f029cddff813` after #2913 merged externally. + Coordinator evidence reports npm dist-tags still at rc.36 / latest 13.0.4, + with no new package published. Maintain a **global release/merge hold** + until all four external consumer pins land and an authorized human configures and + validates the actually required trusted human-approval gate. This work does + not undo #2913's merge, authorize a release, or change either PR. + +Main snapshots are evidence at a point in time, not moving claims. The consumer +bases were refreshed to adcp `1467e46117e8d329f4116e548e9290be75d8e3b3` and +client `55829081dae2673f73615af2a4f6f029cddff813`. PR bodies record their exact +base/head/tree and CI/audit evidence. Other open PRs, including adcp #7450/#7462, +retain their older-base evidence until individually refreshed; this adoption +makes no claim to have recomputed their merge trees. + +## Required administrator action (not executed here) + +An explicitly authorized repository/organization administrator must: + +1. Export the **effective** main rules from repository Settings → Rules → + Rulesets, inherited organization rules, classic branch protection, and all + bypass lists. Record rule IDs, branch patterns, enforcement, expected check + producer App IDs, CODEOWNERS coverage and current permissions. Determine + exactly how #7521's author was permitted to merge. Inspect admin/custom-role, + write/maintain-role, integration, merge-queue and direct-push bypasses. +2. Install a separately reviewed, trusted producer for required status + `Human review / exact head`. Run trusted base or independent App code only; + never PR-head code with write credentials. Require its specific producer + App identity, not just a spoofable context name. Require the policy's Ladon + blocking result as well, with explicit handling of workflow-modification + human holds and high-risk exceptions; optional failures do not block merge. +3. The human producer must paginate reviews, resolve the latest effective + non-dismissed review per person, and count only APPROVED at the **live exact + head** from currently authorized collaborators/required CODEOWNERS. Exclude + the PR author, all Apps/bots, and known automation accounts typed as User. + Check current collaborator permissions, required teams and CODEOWNERS at the + trusted base; fail closed on unknown identities, missing authority, partial + data or API failure. Requested reviewers alone never count. +4. Recompute on push/synchronize, ready-for-review, review submission/dismissal, + base/policy/CODEOWNERS and membership/permission changes; handle merge queues + explicitly. Serialize per PR, re-read head and review/authority state before + publishing, reject stale workers, and invalidate success when any input + changes. Combine with native stale-review dismissal and last-push approval. + Status webhooks are asynchronous: document and test the dismissal/merge + race; if atomic enforcement is not established, retain the operational hold + and use an authorized, trusted merge procedure that revalidates immediately. +5. Make the requirement **non-bypassable by ordinary authors**, including the + author role implicated by #7521. Remove broad role/App bypasses or constrain + them to an independently controlled, logged emergency procedure unavailable + to ordinary authors. Prevent direct-push and alternative merge entrypoints + from evading the same requirement. Settings changes require explicit human + authority; neither this PR nor green CI grants that authority. +6. Record controlled validation under both ordinary author and maintainer roles. + Begin on a non-production branch with matching effective protections and no + deployment/release hooks; any actual merge attempts need separate explicit + administrator authorization. Prove rejection for no reviews, bot-only + approval, COMMENT-only escalation (#7521), requested-but-absent review, + self/old-head/dismissed approval, lost collaborator/CODEOWNER authority, + failed/skipped/missing human status, API failure, stale worker, fork, + workflow-modifying PR, rebase and merge queue. Exercise concurrent push, + dismissal and auto-merge; an obsolete success must not permit merge. A + valid current-head authorized **non-author human** approval may satisfy only + the human gate; unresolved blocking Ladon findings must remain blocking. +7. Verify the actual main branch's effective rules and producer identity after + the controlled tests. Record settings exports, exact PR/head/status/reviewer + evidence and permission-denied merge results, including denial of the + #7521 author scenario. A sandbox-only test is not proof of main enforcement. + Lift the hold only with a named human administrator's recorded acceptance. + +No ruleset, branch setting, tag, merge, deployment or release mutation is part of +these Draft adoption PRs. Do not report branch protection fixed until the +consumer deployments and administrator enforcement/validation are complete. diff --git a/ladon/INSTALL.md b/ladon/INSTALL.md index 7354383..2177aaa 100644 --- a/ladon/INSTALL.md +++ b/ladon/INSTALL.md @@ -25,9 +25,9 @@ review itself. Before the workflow can run: | `SECRETARIAT_APP_PRIVATE_KEY` | The App's private key (PEM) | | `ANTHROPIC_API_KEY` | Anthropic API key Ladon reviews with | -3. **Choose the approval policy explicitly.** Existing consumers default to - `auto-approve: 'true'`, which allows App approvals to satisfy GitHub's review - count. For human-only repositories set `auto-approve: 'false'` as shown +3. **Choose the approval policy explicitly.** Revised review/setup/arbiter + actions default to `auto-approve: 'false'`. Explicit true enables App approval + and is forbidden in this human-only rollout. Set `auto-approve: 'false'` as shown below: clean reviews become COMMENT, blocking findings still request changes and fail, and escalations remain failed pending human resolution. This input does not itself enforce who may satisfy branch protection. Follow the @@ -43,8 +43,7 @@ new input. See the adoption order before using the floating tag. ## Step 1: add the workflow Create `.github/workflows/ai-review.yml`. This is the canonical consumer -workflow: it pins the floating major tag `@ladon/review/v1` (auto-tracks the -latest v1), and it carries the security posture Ladon depends on. **Copy it +workflow: it pins the reviewed immutable orchestrator and carries the security posture Ladon depends on. **Copy it verbatim**: do not reconstruct it (see [Security posture](#security-posture)). ```yaml @@ -140,7 +139,7 @@ jobs: - name: Run Ladon if: steps.workflow-mod.outputs.modified != 'true' - uses: adcontextprotocol/actions/ladon/review@ladon/review/v1 + uses: adcontextprotocol/actions/ladon/review@3d180c9b365c201d4cd4cfd93d0c2a7e8790da70 with: auto-approve: "false" anthropic-api-key: ${{ secrets.ANTHROPIC_API_KEY }} diff --git a/ladon/README.md b/ladon/README.md index c3bc210..1fa17bd 100644 --- a/ladon/README.md +++ b/ladon/README.md @@ -27,8 +27,8 @@ bot identity (the App whose credentials the consuming workflow supplies). See ## Human-only approval Set `auto-approve: 'false'` on the orchestrator to post findings without ever -submitting an approving review, including stale reapproval. The default remains -`'true'` for compatibility. Blocking findings and escalation still fail in the +submitting an approving review, including stale reapproval. All three action manifests default to `'false'`; +only explicit `'true'` enables the compatibility approval path. Blocking findings and escalation still fail in the new mode. See [the adoption plan](./HUMAN-APPROVAL-ADOPTION.md) for consumer pins, auto-merge races, and the separate required human-review/ruleset audit. @@ -47,4 +47,4 @@ and supply the App credentials and Anthropic key as inputs. See [`INSTALL.md`](./INSTALL.md) for the consumer install guide (prerequisites, a copy-paste workflow, and the `pull_request_target` security posture); the repo root `.github/workflows/ai-review.yml` is the self-review variant that uses the -local `./ladon/review` path. +same immutable reviewed action pin. diff --git a/ladon/consumer-policy.json b/ladon/consumer-policy.json new file mode 100644 index 0000000..ebb7c2e --- /dev/null +++ b/ladon/consumer-policy.json @@ -0,0 +1,10 @@ +{ + "review": "3d180c9b365c201d4cd4cfd93d0c2a7e8790da70", + "repositories": [ + "adcontextprotocol/actions", + "adcontextprotocol/adcp", + "adcontextprotocol/adcp-client", + "adcontextprotocol/adcp-go", + "adcontextprotocol/adcp-client-python" + ] +} diff --git a/scripts/audit-ladon-consumers.cjs b/scripts/audit-ladon-consumers.cjs new file mode 100644 index 0000000..7702661 --- /dev/null +++ b/scripts/audit-ladon-consumers.cjs @@ -0,0 +1,243 @@ +#!/usr/bin/env node +// Read-only release prerequisite. Uses live, exact default-branch snapshots, +// not code search alone (the search index can lag). Never changes GitHub state. +const assert = require("node:assert/strict"); +const { execFile } = require("node:child_process"); +const { createHash } = require("node:crypto"); +const { readFileSync } = require("node:fs"); +const { promisify } = require("node:util"); +const { parse } = require("yaml"); +const exec = promisify(execFile); +const policy = JSON.parse( + readFileSync( + require("node:path").join(__dirname, "../ladon/consumer-policy.json"), + "utf8", + ), +); + +function invocations(value, path = "", sites = []) { + if (!value || typeof value !== "object") return sites; + if (typeof value.uses === "string" && /ladon/i.test(value.uses)) { + sites.push({ + path, + uses: value.uses, + autoApprove: value.with?.["auto-approve"], + }); + } + for (const [key, child] of Object.entries(value)) + invocations(child, `${path}.${key}`, sites); + return sites; +} + +function validateInventory(report, expected = policy) { + const errors = []; + if (report.complete !== true) + errors.push("Incomplete organization inventory"); + if (report.search?.incomplete_results !== false) + errors.push("Incomplete code search"); + if (report.search?.total_count !== report.search?.items?.length) + errors.push("Truncated code search"); + const observed = new Map(); + for (const repo of report.repositories || []) { + if (observed.has(repo.name)) + errors.push(`Duplicate repository: ${repo.name}`); + observed.set(repo.name, repo); + if (!/^[a-f0-9]{40}$/.test(repo.head || "")) + errors.push(`Missing exact head: ${repo.name}`); + const sites = (repo.workflows || []).flatMap((file) => + invocations(parse(file.content), file.path), + ); + if (sites.length && !expected.repositories.includes(repo.name)) + errors.push(`Uninventoried Ladon consumer: ${repo.name}`); + if (expected.repositories.includes(repo.name) && sites.length !== 1) + errors.push( + `${repo.name}: expected one reviewed invocation, found ${sites.length}`, + ); + for (const site of sites) { + if ( + site.uses !== + `adcontextprotocol/actions/ladon/review@${expected.review}` + ) + errors.push(`${repo.name}:${site.path}: missing/wrong immutable pin`); + if (site.autoApprove !== "false") + errors.push( + `${repo.name}:${site.path}: auto-approve must be literal 'false'`, + ); + } + } + for (const name of expected.repositories) + if (!observed.has(name)) + errors.push(`Missing consumer repository: ${name}`); + for (const item of report.search?.items || []) { + const repo = observed.get(item.repository.full_name); + if (!repo?.workflows.some((file) => file.path === item.path)) + errors.push( + `Unreconciled code-search result: ${item.repository.full_name}:${item.path}`, + ); + } + return errors; +} + +async function api(path, optional404 = false) { + try { + const { stdout } = await exec("gh", ["api", "--method", "GET", path], { + maxBuffer: 16 * 1024 * 1024, + timeout: 60000, + }); + return JSON.parse(stdout); + } catch (error) { + if (optional404 && /HTTP 404/.test(error.stderr || "")) return null; + throw error; + } +} + +async function pages(path) { + const items = []; + for (let page = 1; ; page++) { + const result = await api( + `${path}${path.includes("?") ? "&" : "?"}per_page=100&page=${page}`, + ); + assert.ok(Array.isArray(result), "Expected a complete paginated API list"); + items.push(...result); + if (result.length < 100) return items; + } +} + +async function mapLimited(items, fn) { + const result = new Array(items.length); + let next = 0; + await Promise.all( + Array.from({ length: Math.min(6, items.length) }, async () => { + while (next < items.length) { + const index = next++; + result[index] = await fn(items[index]); + } + }), + ); + return result; +} + +async function audit(actionsHead) { + if (actionsHead !== undefined) + assert.match( + actionsHead, + /^[a-f0-9]{40}$/, + "Candidate must be an exact actions commit", + ); + const query = encodeURIComponent( + 'org:adcontextprotocol "ladon" path:.github/workflows', + ); + const search = await api(`search/code?q=${query}&per_page=100`); + assert.equal(search.incomplete_results, false, "Code search is incomplete"); + assert.ok( + search.total_count <= 100, + "Code search exceeds the audited pagination bound; expand audit explicitly", + ); + assert.equal( + search.items.length, + search.total_count, + "Code search is truncated", + ); + const repositories = await pages("orgs/adcontextprotocol/repos?type=all"); + const snapshots = await mapLimited( + repositories.filter((repo) => !repo.archived && !repo.disabled), + async (repo) => { + const commit = await api( + `repos/${repo.full_name}/commits/${encodeURIComponent(repo.default_branch)}`, + ); + const snapshot = + actionsHead && repo.full_name === "adcontextprotocol/actions" + ? await api(`repos/${repo.full_name}/commits/${actionsHead}`) + : commit; + const files = await api( + `repos/${repo.full_name}/contents/.github/workflows?ref=${snapshot.sha}`, + true, + ); + assert.ok( + files === null || Array.isArray(files), + `Invalid workflows directory: ${repo.full_name}`, + ); + const workflows = await mapLimited( + (files || []).filter((file) => /\.ya?ml$/.test(file.path)), + async (file) => { + assert.equal(file.type, "file", `Non-file workflow: ${file.path}`); + const blob = await api( + `repos/${repo.full_name}/git/blobs/${file.sha}`, + ); + assert.equal(blob.encoding, "base64"); + const bytes = Buffer.from(blob.content, "base64"); + const hash = createHash("sha1") + .update(`blob ${bytes.length}\0`) + .update(bytes) + .digest("hex"); + assert.equal( + hash, + file.sha, + "Workflow bytes do not match exact Git blob", + ); + return { + path: file.path, + blob: file.sha, + content: bytes.toString("utf8"), + }; + }, + ); + return { + name: repo.full_name, + head: snapshot.sha, + liveHead: commit.sha, + tree: snapshot.commit.tree.sha, + workflows, + }; + }, + ); + // Reject head movement during enumeration; never certify a mixed snapshot. + for (const repo of snapshots) { + const branch = repositories.find( + (item) => item.full_name === repo.name, + ).default_branch; + const current = await api( + `repos/${repo.name}/commits/${encodeURIComponent(branch)}`, + ); + assert.equal( + current.sha, + repo.liveHead, + `Main moved during audit: ${repo.name}`, + ); + } + const report = { + at: new Date().toISOString(), + scope: actionsHead + ? "pre-publication actions candidate; NOT live enforcement" + : "live default branches", + actionsCandidate: actionsHead, + complete: true, + search, + repositories: snapshots, + }; + report.errors = validateInventory(report); + return report; +} + +module.exports = { invocations, validateInventory, audit }; +if (require.main === module) { + const args = process.argv.slice(2); + if (args.length && (args.length !== 2 || args[0] !== "--actions-head")) + throw new Error( + "Usage: audit-ladon-consumers.cjs [--actions-head EXACT_SHA]", + ); + audit(args[1]) + .then((report) => { + process.stdout.write(`${JSON.stringify(report, null, 2)}\n`); + if (report.errors.length) { + console.error( + `HOLD: ${report.errors.length} unsafe or unreviewed Ladon inventory entries`, + ); + process.exitCode = 1; + } + }) + .catch((error) => { + console.error(`HOLD: inventory could not be verified: ${error.message}`); + process.exitCode = 1; + }); +} From d2422f6b24f6c0b7535c1155e9175d388be44174 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 14 Sep 2026 18:23:06 +0000 Subject: [PATCH 09/11] fix(ladon): reject whitespace in approval capabilities --- .github/workflows/pull-request.yml | 3 ++- internal/versioning/ladon-consumers.test.ts | 15 +++++++++++ ladon/arbiter/dist/index.js | 5 +++- ladon/arbiter/src/index.ts | 5 +++- .../src/no-approval.integration.test.ts | 12 ++++++++- ladon/setup/dist/index.js | 5 +++- ladon/setup/src/index.ts | 5 +++- .../setup/src/no-approval.integration.test.ts | 8 +++++- scripts/audit-ladon-consumers.cjs | 27 ++++++++++++++----- 9 files changed, 72 insertions(+), 13 deletions(-) diff --git a/.github/workflows/pull-request.yml b/.github/workflows/pull-request.yml index c23a290..c0948da 100644 --- a/.github/workflows/pull-request.yml +++ b/.github/workflows/pull-request.yml @@ -40,4 +40,5 @@ jobs: node-version-file: .nvmrc cache: npm - run: npm ci - - run: npx turbo run test --filter="...[${{ env.TURBO_FILTER }}]" + # Inventory/manifest changes outside a package must exercise all gates. + - run: npm test -- --force diff --git a/internal/versioning/ladon-consumers.test.ts b/internal/versioning/ladon-consumers.test.ts index b5451a1..4ae24f4 100644 --- a/internal/versioning/ladon-consumers.test.ts +++ b/internal/versioning/ladon-consumers.test.ts @@ -156,3 +156,18 @@ describe('organization Ladon release inventory is fail closed', () => { ) }) }) + +test('the Contents API cap cannot hide a sixth consumer beyond a truncated listing', () => { + const { checkedWorkflowFiles } = createRequire(import.meta.url)( + '../../scripts/audit-ladon-consumers.cjs', + ) + const files = Array.from({ length: 1000 }, (_, i) => ({ + path: `.github/workflows/non-yaml-${i}`, + })) + expect(() => checkedWorkflowFiles(files, 'adcontextprotocol/sixth')).toThrow( + 'Possibly truncated', + ) + expect( + checkedWorkflowFiles(files.slice(0, 999), 'adcontextprotocol/sixth'), + ).toEqual([]) +}) diff --git a/ladon/arbiter/dist/index.js b/ladon/arbiter/dist/index.js index 00009ac..9a3252f 100644 --- a/ladon/arbiter/dist/index.js +++ b/ladon/arbiter/dist/index.js @@ -48043,7 +48043,10 @@ function changeRequestIds(input) { async function main() { // The manifest supplies the compatible default. Empty/invalid explicit input // must fail before any API write, rather than enabling approvals by fallback. - const autoApproveInput = getInput('auto-approve', { required: true }); + const autoApproveInput = getInput('auto-approve', { + required: true, + trimWhitespace: false, + }); if (autoApproveInput !== 'true' && autoApproveInput !== 'false') { throw new Error('auto-approve must be exactly "true" or "false"'); } diff --git a/ladon/arbiter/src/index.ts b/ladon/arbiter/src/index.ts index 0fa333a..85b5dee 100644 --- a/ladon/arbiter/src/index.ts +++ b/ladon/arbiter/src/index.ts @@ -49,7 +49,10 @@ function changeRequestIds(input: string): number[] { async function main(): Promise { // The manifest supplies the compatible default. Empty/invalid explicit input // must fail before any API write, rather than enabling approvals by fallback. - const autoApproveInput = core.getInput('auto-approve', { required: true }) + const autoApproveInput = core.getInput('auto-approve', { + required: true, + trimWhitespace: false, + }) if (autoApproveInput !== 'true' && autoApproveInput !== 'false') { throw new Error('auto-approve must be exactly "true" or "false"') } diff --git a/ladon/arbiter/src/no-approval.integration.test.ts b/ladon/arbiter/src/no-approval.integration.test.ts index bab3698..dd5274b 100644 --- a/ladon/arbiter/src/no-approval.integration.test.ts +++ b/ladon/arbiter/src/no-approval.integration.test.ts @@ -8,7 +8,10 @@ const fixture = vi.hoisted(() => { const inputs: Record = {} const defaults: Record = {} const core = { - getInput: vi.fn((name: string) => inputs[name] ?? defaults[name] ?? ''), + getInput: vi.fn((name: string, options?: { trimWhitespace?: boolean }) => { + const raw = inputs[name] ?? defaults[name] ?? '' + return options?.trimWhitespace === false ? raw : raw.trim() + }), setOutput: vi.fn(), setFailed: vi.fn(), warning: vi.fn(), @@ -272,12 +275,19 @@ describe('arbiter API and action result integration', () => { 'disabled', '${{ inputs.auto-approve }}', ' false', + ' true', + 'true ', + '\ttrue\n', 'null', ])( 'invalid auto-approve=%j fails before any review API call', async (input) => { fixture.inputs['auto-approve'] = input await run() + expect(fixture.core.getInput).toHaveBeenCalledWith('auto-approve', { + required: true, + trimWhitespace: false, + }) expect(fixture.core.setFailed).toHaveBeenCalled() expect(fixture.decide).not.toHaveBeenCalled() expect(fixture.octokit.rest.pulls.createReview).not.toHaveBeenCalled() diff --git a/ladon/setup/dist/index.js b/ladon/setup/dist/index.js index 6ebb07b..1e990b9 100644 --- a/ladon/setup/dist/index.js +++ b/ladon/setup/dist/index.js @@ -39945,7 +39945,10 @@ async function fetchReviewDecision(params) { async function main() { // The manifest supplies the compatible default. Empty/invalid explicit input // must fail before any API write, rather than enabling approvals by fallback. - const autoApproveInput = getInput('auto-approve', { required: true }); + const autoApproveInput = getInput('auto-approve', { + required: true, + trimWhitespace: false, + }); if (autoApproveInput !== 'true' && autoApproveInput !== 'false') { throw new Error('auto-approve must be exactly "true" or "false"'); } diff --git a/ladon/setup/src/index.ts b/ladon/setup/src/index.ts index 8533fe2..8916bb9 100644 --- a/ladon/setup/src/index.ts +++ b/ladon/setup/src/index.ts @@ -165,7 +165,10 @@ async function fetchReviewDecision(params: { async function main(): Promise { // The manifest supplies the compatible default. Empty/invalid explicit input // must fail before any API write, rather than enabling approvals by fallback. - const autoApproveInput = core.getInput('auto-approve', { required: true }) + const autoApproveInput = core.getInput('auto-approve', { + required: true, + trimWhitespace: false, + }) if (autoApproveInput !== 'true' && autoApproveInput !== 'false') { throw new Error('auto-approve must be exactly "true" or "false"') } diff --git a/ladon/setup/src/no-approval.integration.test.ts b/ladon/setup/src/no-approval.integration.test.ts index a8c48a5..a9df61a 100644 --- a/ladon/setup/src/no-approval.integration.test.ts +++ b/ladon/setup/src/no-approval.integration.test.ts @@ -9,7 +9,10 @@ const fixture = vi.hoisted(() => { const defaults: Record = {} const outputs: Record = {} const core = { - getInput: vi.fn((name: string) => inputs[name] ?? defaults[name] ?? ''), + getInput: vi.fn((name: string, options?: { trimWhitespace?: boolean }) => { + const raw = inputs[name] ?? defaults[name] ?? '' + return options?.trimWhitespace === false ? raw : raw.trim() + }), setOutput: vi.fn((name: string, value: string) => { outputs[name] = value }), @@ -227,6 +230,9 @@ describe('setup stale-approval integration', () => { 'off', '${{ inputs.auto-approve }}', ' false', + ' true', + 'true ', + '\ttrue\n', 'null', ])('invalid input %j fails closed', async (input) => { fixture.inputs['auto-approve'] = input diff --git a/scripts/audit-ladon-consumers.cjs b/scripts/audit-ladon-consumers.cjs index 7702661..705267c 100644 --- a/scripts/audit-ladon-consumers.cjs +++ b/scripts/audit-ladon-consumers.cjs @@ -117,6 +117,20 @@ async function mapLimited(items, fn) { return result; } +function checkedWorkflowFiles(files, repository) { + assert.ok( + files === null || Array.isArray(files), + `Invalid workflows directory: ${repository}`, + ); + // Contents API caps directory responses at 1,000 entries. Search can lag, + // so accepting this boundary could hide a newly added sixth consumer. + assert.ok( + files === null || files.length < 1000, + `Possibly truncated workflows directory: ${repository}`, + ); + return (files || []).filter((file) => /\.ya?ml$/.test(file.path)); +} + async function audit(actionsHead) { if (actionsHead !== undefined) assert.match( @@ -153,12 +167,8 @@ async function audit(actionsHead) { `repos/${repo.full_name}/contents/.github/workflows?ref=${snapshot.sha}`, true, ); - assert.ok( - files === null || Array.isArray(files), - `Invalid workflows directory: ${repo.full_name}`, - ); const workflows = await mapLimited( - (files || []).filter((file) => /\.ya?ml$/.test(file.path)), + checkedWorkflowFiles(files, repo.full_name), async (file) => { assert.equal(file.type, "file", `Non-file workflow: ${file.path}`); const blob = await api( @@ -219,7 +229,12 @@ async function audit(actionsHead) { return report; } -module.exports = { invocations, validateInventory, audit }; +module.exports = { + invocations, + validateInventory, + checkedWorkflowFiles, + audit, +}; if (require.main === module) { const args = process.argv.slice(2); if (args.length && (args.length !== 2 || args[0] !== "--actions-head")) From d6e930d4a2a01ee0d476fd8347eaf258f85d889c Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 14 Sep 2026 18:23:52 +0000 Subject: [PATCH 10/11] fix(ladon): pin strict approval input validation --- ladon/review/action.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ladon/review/action.yml b/ladon/review/action.yml index 946fa68..f9a915b 100644 --- a/ladon/review/action.yml +++ b/ladon/review/action.yml @@ -97,7 +97,7 @@ runs: # Update these pins explicitly when promoting a reviewed sub-action change. - name: Setup id: setup - uses: adcontextprotocol/actions/ladon/setup@02db55f54f39f93c683c2031887b735aea0ecba2 + uses: adcontextprotocol/actions/ladon/setup@d2422f6b24f6c0b7535c1155e9175d388be44174 with: github-token: ${{ steps.app-token.outputs.token }} ladon-bot-login: ${{ steps.app-token.outputs.app-slug }}[bot] @@ -162,7 +162,7 @@ runs: - name: Arbiter if: steps.setup.outputs.should-run == 'true' id: arbiter - uses: adcontextprotocol/actions/ladon/arbiter@02db55f54f39f93c683c2031887b735aea0ecba2 + uses: adcontextprotocol/actions/ladon/arbiter@d2422f6b24f6c0b7535c1155e9175d388be44174 with: anthropic-api-key: ${{ inputs.anthropic-api-key }} github-token: ${{ steps.app-token.outputs.token }} From cd51758e85420bb36400c911462c18c629c27382 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 14 Sep 2026 18:24:13 +0000 Subject: [PATCH 11/11] fix(ladon): coordinate strict default-deny consumer pins --- .github/workflows/ai-review.yml | 2 +- ladon/HUMAN-APPROVAL-ADOPTION.md | 8 ++++---- ladon/INSTALL.md | 2 +- ladon/consumer-policy.json | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 931d54a..6634a13 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -87,7 +87,7 @@ jobs: - name: Run Ladon if: steps.workflow-mod.outputs.modified != 'true' - uses: adcontextprotocol/actions/ladon/review@3d180c9b365c201d4cd4cfd93d0c2a7e8790da70 + uses: adcontextprotocol/actions/ladon/review@d6e930d4a2a01ee0d476fd8347eaf258f85d889c with: auto-approve: 'false' anthropic-api-key: ${{ secrets.ANTHROPIC_API_KEY }} diff --git a/ladon/HUMAN-APPROVAL-ADOPTION.md b/ladon/HUMAN-APPROVAL-ADOPTION.md index d03e4e9..e985e6b 100644 --- a/ladon/HUMAN-APPROVAL-ADOPTION.md +++ b/ladon/HUMAN-APPROVAL-ADOPTION.md @@ -31,7 +31,7 @@ confirm the credential can see the entire intended organization. `ladon/consumer-policy.json` is the explicit inventory. The deterministic tests reject a sixth consumer, missing expected consumers, added invocation sites, wrong/local/floating pins, missing/nonliteral/true inputs, malformed YAML, -partial search results and incomplete enumeration. No approving consumer is +partial search results, capped Contents listings and incomplete enumeration. No approving consumer is accepted. Explicit true compatibility is tested solely as a supported opt-in; adding an approving consumer would require separate human policy acceptance, recorded inventory and a reviewed change to this enforcement contract. @@ -39,9 +39,9 @@ recorded inventory and a reviewed change to this enforcement contract. ## Reviewed immutable implementation - Orchestrator and approval-boundary regression suite: - `3d180c9b365c201d4cd4cfd93d0c2a7e8790da70`. + `d6e930d4a2a01ee0d476fd8347eaf258f85d889c`. - Nested setup/arbiter manifests, source and executable bundles: - `02db55f54f39f93c683c2031887b735aea0ecba2`. + `d2422f6b24f6c0b7535c1155e9175d388be44174`. - Nested reviewer: `a64a17ba369122d6b3401f614a31df7b8607f043`. All five proposed workflows pin that orchestrator and set `auto-approve: 'false'` @@ -55,7 +55,7 @@ existing modification gate. Do not revive it as a cleanup-based approval guard. Unlike superseded head `85284cbc297875aa04b2d5afcf7ae833b29f329c`, this artifact **defaults false in all three manifests**. Direct helper omissions also deny -approval/shortcuts. Raw empty/malformed/nonliteral inputs fail before reviews; +approval/shortcuts. Raw empty/malformed/nonliteral inputs (including whitespace) fail before reviews; composite validation precedes App token minting. Tests cover runner-injected omission defaults and missing raw input, with zero APPROVE/reapprove API calls. The new default is a behavior change: legacy approval requires explicit true. diff --git a/ladon/INSTALL.md b/ladon/INSTALL.md index 2177aaa..7457a06 100644 --- a/ladon/INSTALL.md +++ b/ladon/INSTALL.md @@ -139,7 +139,7 @@ jobs: - name: Run Ladon if: steps.workflow-mod.outputs.modified != 'true' - uses: adcontextprotocol/actions/ladon/review@3d180c9b365c201d4cd4cfd93d0c2a7e8790da70 + uses: adcontextprotocol/actions/ladon/review@d6e930d4a2a01ee0d476fd8347eaf258f85d889c with: auto-approve: "false" anthropic-api-key: ${{ secrets.ANTHROPIC_API_KEY }} diff --git a/ladon/consumer-policy.json b/ladon/consumer-policy.json index ebb7c2e..29e1998 100644 --- a/ladon/consumer-policy.json +++ b/ladon/consumer-policy.json @@ -1,5 +1,5 @@ { - "review": "3d180c9b365c201d4cd4cfd93d0c2a7e8790da70", + "review": "d6e930d4a2a01ee0d476fd8347eaf258f85d889c", "repositories": [ "adcontextprotocol/actions", "adcontextprotocol/adcp",