diff --git a/__tests__/e2e/webhook.test.js b/__tests__/e2e/webhook.test.js index 8de38e3..3cb8e93 100644 --- a/__tests__/e2e/webhook.test.js +++ b/__tests__/e2e/webhook.test.js @@ -319,10 +319,14 @@ describe('E2E webhook simulation', () => { // The webhook signature is valid so Probot accepts it. The handler will // attempt GitHub API calls (e.g. checking org membership) which fail - // without a real connection, so Probot may return 500 from the handler - // error. 200/202 (accepted) and 500 (handler error) all confirm the - // signature was validated and the event was dispatched to the handler. - expect([200, 202, 500]).toContain(res.status); + // without a real installation. Probot returns one of: + // 200/202: signature validated, handler completed (handler may have + // silently returned without making API calls) + // 401: handler proceeded and tried to acquire an installation + // token but credentials are fake + // 500: unhandled exception inside the handler + // All four confirm the signature was validated and the event dispatched. + expect([200, 202, 401, 500]).toContain(res.status); }, 15000); // ----------------------------------------------------------------------- diff --git a/__tests__/integration/ai-review.test.js b/__tests__/integration/ai-review.test.js index 47608a7..c792031 100644 --- a/__tests__/integration/ai-review.test.js +++ b/__tests__/integration/ai-review.test.js @@ -629,6 +629,25 @@ describe('ai-review', () => { const callArgs = global.fetch.mock.calls[0]; expect(callArgs[1].signal).toBeInstanceOf(AbortSignal); }); + + it('passes responseFormat through to the wire payload as response_format', async () => { + mockFetchSuccess('{"verdict":"approve","summary":"x","findings":[]}'); + const fmt = { type: 'json_schema', json_schema: { name: 'X', strict: true, schema: { type: 'object' } } }; + + await callLocalAI(mockEndpoint, mockModel, mockSystemPrompt, mockUserPrompt, { + responseFormat: fmt + }); + + const body = JSON.parse(global.fetch.mock.calls[0][1].body); + expect(body.response_format).toEqual(fmt); + }); + + it('omits response_format when not requested (legacy compat)', async () => { + mockFetchSuccess('plain prose'); + await callLocalAI(mockEndpoint, mockModel, mockSystemPrompt, mockUserPrompt); + const body = JSON.parse(global.fetch.mock.calls[0][1].body); + expect(body.response_format).toBeUndefined(); + }); }); // ========================================================================= diff --git a/__tests__/integration/app.test.js b/__tests__/integration/app.test.js index 92cd4fc..2b68486 100644 --- a/__tests__/integration/app.test.js +++ b/__tests__/integration/app.test.js @@ -137,17 +137,41 @@ function setupApp(options = {}) { } function createMockOctokit() { + // Production code now goes through octokit.request() for all issue ops via + // the issueOps helper (was octokit.issues.X — that namespace turned out to + // be undefined at runtime for both pull_request.opened and + // issue_comment.created in Probot v14). Existing assertions still target + // .issues.X jest.fn()s; route the request mock into them by route so + // assertions keep working unchanged. + const issuesCreate = jest.fn().mockResolvedValue({ data: { number: 42 } }); + const issuesCreateComment = jest.fn().mockResolvedValue({ data: { id: 100 } }); + const issuesUpdateComment = jest.fn().mockResolvedValue({}); + const issuesDeleteComment = jest.fn().mockResolvedValue({}); + + const routeDispatch = { + 'POST /repos/{owner}/{repo}/issues': issuesCreate, + 'POST /repos/{owner}/{repo}/issues/{issue_number}/comments': issuesCreateComment, + 'PATCH /repos/{owner}/{repo}/issues/comments/{comment_id}': issuesUpdateComment, + 'DELETE /repos/{owner}/{repo}/issues/comments/{comment_id}': issuesDeleteComment + }; + + const request = jest.fn((route, params) => { + const dispatch = routeDispatch[route]; + if (dispatch) return dispatch(params); + return Promise.resolve({ status: 200, data: {} }); + }); + return { issues: { - create: jest.fn().mockResolvedValue({ data: { number: 42 } }), - createComment: jest.fn().mockResolvedValue({ data: { id: 100 } }), - updateComment: jest.fn().mockResolvedValue({}), - deleteComment: jest.fn().mockResolvedValue({}) + create: issuesCreate, + createComment: issuesCreateComment, + updateComment: issuesUpdateComment, + deleteComment: issuesDeleteComment }, repos: { getBranch: jest.fn().mockResolvedValue({ data: { name: 'main' } }) }, - request: jest.fn().mockResolvedValue({ status: 200, data: {} }) + request }; } @@ -1105,7 +1129,11 @@ describe('app', () => { const { handlers } = setupApp(); const context = createIssueCommentContext('/generate-dependabot'); - context.octokit.request.mockResolvedValue({ data: { default_branch: 'main' } }); + // mockImplementationOnce so the dispatcher in createMockOctokit still + // routes subsequent createComment calls into the namespaced jest.fn. + context.octokit.request.mockImplementationOnce(() => + Promise.resolve({ data: { default_branch: 'main' } }) + ); await handlers['issue_comment.created'](context); expect(generateDependabotConfig).toHaveBeenCalledWith(context.octokit, 'myorg', 'myrepo', 'main'); @@ -1123,7 +1151,11 @@ describe('app', () => { const { handlers } = setupApp(); const context = createIssueCommentContext('/generate-dependabot'); - context.octokit.request.mockResolvedValue({ data: { default_branch: 'main' } }); + // mockImplementationOnce so the dispatcher in createMockOctokit still + // routes subsequent createComment calls into the namespaced jest.fn. + context.octokit.request.mockImplementationOnce(() => + Promise.resolve({ data: { default_branch: 'main' } }) + ); await handlers['issue_comment.created'](context); expect(applyDependabotConfig).not.toHaveBeenCalled(); @@ -1136,7 +1168,11 @@ describe('app', () => { const { handlers } = setupApp(); const context = createIssueCommentContext('/generate-dependabot'); - context.octokit.request.mockResolvedValue({ data: { default_branch: 'main' } }); + // mockImplementationOnce so the dispatcher in createMockOctokit still + // routes subsequent createComment calls into the namespaced jest.fn. + context.octokit.request.mockImplementationOnce(() => + Promise.resolve({ data: { default_branch: 'main' } }) + ); await handlers['issue_comment.created'](context); const body = context.octokit.issues.createComment.mock.calls[0][0].body; diff --git a/__tests__/unit/ai-review-prompt.test.js b/__tests__/unit/ai-review-prompt.test.js index 4502880..727c338 100644 --- a/__tests__/unit/ai-review-prompt.test.js +++ b/__tests__/unit/ai-review-prompt.test.js @@ -1,5 +1,7 @@ import { STRICT_SYSTEM_PROMPT, + REVIEW_JSON_SCHEMA, + buildResponseFormat, isSlop, tryParseReview, filterFindings, @@ -16,6 +18,40 @@ describe('STRICT_SYSTEM_PROMPT', () => { }); }); +describe('REVIEW_JSON_SCHEMA', () => { + it('declares the verdict enum exactly matching the parser', () => { + expect(REVIEW_JSON_SCHEMA.properties.verdict.enum).toEqual([ + 'approve', 'comment', 'request_changes' + ]); + }); + + it('marks verdict, summary, findings as required', () => { + expect(REVIEW_JSON_SCHEMA.required.sort()).toEqual( + ['findings', 'summary', 'verdict'] + ); + }); + + it('rejects extra properties (additionalProperties: false)', () => { + expect(REVIEW_JSON_SCHEMA.additionalProperties).toBe(false); + expect(REVIEW_JSON_SCHEMA.properties.findings.items.additionalProperties).toBe(false); + }); + + it('every finding field used by the parser is declared in the schema', () => { + const fields = Object.keys(REVIEW_JSON_SCHEMA.properties.findings.items.properties).sort(); + expect(fields).toEqual(['claim', 'file', 'line', 'quoted_line']); + }); +}); + +describe('buildResponseFormat', () => { + it('returns OpenAI-compat json_schema payload', () => { + const rf = buildResponseFormat(); + expect(rf.type).toBe('json_schema'); + expect(rf.json_schema.name).toBe('PullRequestReview'); + expect(rf.json_schema.strict).toBe(true); + expect(rf.json_schema.schema).toBe(REVIEW_JSON_SCHEMA); + }); +}); + describe('isSlop', () => { it.each([ 'consider edge cases here', diff --git a/src/ai-review-prompt.js b/src/ai-review-prompt.js index 9963ba5..b5d5807 100644 --- a/src/ai-review-prompt.js +++ b/src/ai-review-prompt.js @@ -50,6 +50,65 @@ Example with a finding: Example with no findings: {"verdict":"approve","summary":"Pure docs change in README.","findings":[]}`; +/** + * JSON Schema describing the strict review payload. Used as a wire-format + * contract: passed to Ollama via OpenAI-compat `response_format` so the + * decoder is grammar-constrained at generation time. The model literally + * cannot emit a token sequence that violates the schema. + * + * Belt-and-suspenders relative to the prompt rules above: the prompt + * teaches the *intent* (banned hedging, quote-or-die etc.) while the schema + * enforces the *shape*. Today's wrong-shape failures (model emitting + * `{"commit_message":"..."}` instead of the review object) become + * impossible. + */ +export const REVIEW_JSON_SCHEMA = { + type: 'object', + additionalProperties: false, + properties: { + verdict: { + type: 'string', + enum: ['approve', 'comment', 'request_changes'] + }, + summary: { + type: 'string', + maxLength: 400 + }, + findings: { + type: 'array', + items: { + type: 'object', + additionalProperties: false, + properties: { + file: { type: 'string', minLength: 1 }, + line: { type: 'integer', minimum: 1 }, + quoted_line: { type: 'string', minLength: 1 }, + claim: { type: 'string', minLength: 5, maxLength: 400 } + }, + required: ['file', 'line', 'quoted_line', 'claim'] + } + } + }, + required: ['verdict', 'summary', 'findings'] +}; + +/** + * Build the OpenAI-compat `response_format` payload for grammar-constrained + * JSON output. Ollama (≥0.5) honours this on /v1/chat/completions; older + * builds may fall back to plain JSON mode or ignore it entirely (in which + * case the parser still catches non-conforming output). + */ +export function buildResponseFormat() { + return { + type: 'json_schema', + json_schema: { + name: 'PullRequestReview', + strict: true, + schema: REVIEW_JSON_SCHEMA + } + }; +} + const HEDGE_OR_SLOP_PATTERNS = [ /\bmight\b/i, /\bcould\b/i, diff --git a/src/ai-review.js b/src/ai-review.js index f4ccd0f..9f2d193 100644 --- a/src/ai-review.js +++ b/src/ai-review.js @@ -8,7 +8,8 @@ import { tryParseReview, filterFindings, computeVerdict, - renderReviewMarkdown + renderReviewMarkdown, + buildResponseFormat } from './ai-review-prompt.js'; import { runRivetOracle } from './rivet-oracle.js'; import { hasRivetYaml, withTempRepoCheckout } from './rivet-fetch.js'; @@ -223,25 +224,38 @@ function buildReviewPrompt(prData, diff, files, maxDiffSize) { } async function callLocalAI(endpoint, model, systemPrompt, userPrompt, options = {}) { - const { maxTokens = 2000, temperature = 0.3, timeout = 300000 } = options; + const { + maxTokens = 2000, + temperature = 0.3, + timeout = 300000, + responseFormat = undefined + } = options; const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), timeout); + const payload = { + model, + messages: [ + { role: 'system', content: systemPrompt }, + { role: 'user', content: userPrompt } + ], + max_tokens: maxTokens, + temperature, + stream: false + }; + if (responseFormat) { + // OpenAI-compat: forces grammar-constrained output. Ollama honours + // this on /v1/chat/completions in recent builds. Older endpoints that + // ignore it fall through to the strict-prompt + parser path. + payload.response_format = responseFormat; + } + try { const response = await fetch(endpoint, { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ - model, - messages: [ - { role: 'system', content: systemPrompt }, - { role: 'user', content: userPrompt } - ], - max_tokens: maxTokens, - temperature, - stream: false - }), + body: JSON.stringify(payload), signal: controller.signal }); @@ -438,7 +452,10 @@ async function reviewPullRequest(octokit, owner, repo, prNumber) { { maxTokens: aiConfig.max_tokens || 2000, temperature: aiConfig.temperature || 0.3, - timeout: aiConfig.timeout || 120000 + timeout: aiConfig.timeout || 120000, + // Grammar-constrained output in strict mode only — legacy freeform + // path expects prose, would get rejected by JSON-schema decoding. + responseFormat: useStrictMode ? buildResponseFormat() : undefined } ); diff --git a/src/app.js b/src/app.js index 8db4fe7..6d2984f 100644 --- a/src/app.js +++ b/src/app.js @@ -45,6 +45,27 @@ function getEnqueueTask() { : null; } +/** + * Octokit's `.issues.X` namespace is unreliable in Probot v14 — frequently + * undefined on `pull_request.opened` AND `issue_comment.created` events + * (see git history of `src/ai-review.js` and PR #22 for the hours of + * production silence this caused). Always go through `.request(route, args)` + * — that primitive is consistently available regardless of event context. + * + * These helpers exist so call sites stay short and the routes are declared + * once, not 28 times. + */ +const issueOps = { + createComment: (octokit, args) => + octokit.request('POST /repos/{owner}/{repo}/issues/{issue_number}/comments', args), + updateComment: (octokit, args) => + octokit.request('PATCH /repos/{owner}/{repo}/issues/comments/{comment_id}', args), + deleteComment: (octokit, args) => + octokit.request('DELETE /repos/{owner}/{repo}/issues/comments/{comment_id}', args), + createIssue: (octokit, args) => + octokit.request('POST /repos/{owner}/{repo}/issues', args) +}; + function applySecurityHeaders(res) { res.setHeader('X-Content-Type-Options', 'nosniff'); res.setHeader('X-Frame-Options', 'SAMEORIGIN'); @@ -165,7 +186,7 @@ function registerApp(app, options = {}) { if (repository.has_issues) { try { - await context.octokit.issues.create({ + await issueOps.createIssue(context.octokit,{ owner, repo: repoName, title: 'Repository Configuration', @@ -220,7 +241,7 @@ function registerApp(app, options = {}) { const repo = repository.name; if (!validation.valid) { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issue.number, @@ -233,7 +254,7 @@ function registerApp(app, options = {}) { const enqueueTask = getEnqueueTask(); if (!enqueueTask) { getLogger().warn('Task store not initialized — cannot enqueue provision-repo task'); - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issue.number, @@ -254,7 +275,7 @@ function registerApp(app, options = {}) { } ); - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issue.number, @@ -313,7 +334,7 @@ function registerApp(app, options = {}) { senderLogin ); if (!isOrgMember) { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -326,7 +347,7 @@ function registerApp(app, options = {}) { const requireAllowedUser = async () => { if (!isAllowedUser) { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -351,7 +372,7 @@ function registerApp(app, options = {}) { enqueueTask: getEnqueueTask() }); - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -371,7 +392,7 @@ function registerApp(app, options = {}) { const targetOrg = config?.organization || owner; const syncResult = await synchronizeAllRepositories(context.octokit, targetOrg); - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -389,7 +410,7 @@ function registerApp(app, options = {}) { } const configReport = await generateConfigurationReport(context.octokit, owner, repo); - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -405,7 +426,7 @@ function registerApp(app, options = {}) { } const dependabotReport = await checkDependabotConfiguration(context.octokit, owner, repo); - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -429,14 +450,14 @@ function registerApp(app, options = {}) { dependabotCheck.labelIssues ); - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, body: `✅ Fixed labels on ${fixResult.fixedIssues} Dependabot PRs!` }); } else { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -460,7 +481,7 @@ function registerApp(app, options = {}) { const result = await generateDependabotConfig(context.octokit, owner, repo, defaultBranch); if (!result.config) { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, body: `No package ecosystems detected in ${owner}/${repo}. Nothing to generate.` }); @@ -471,20 +492,20 @@ function registerApp(app, options = {}) { body += '```yaml\n' + yaml.dump(result.config, { noRefs: true }) + '```\n\n'; body += 'Applying configuration...'; - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, body }); await applyDependabotConfig(context.octokit, owner, repo, result.config); - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, body: '✅ Dependabot configuration applied!' }); } } catch (error) { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, body: `❌ Error generating Dependabot config: ${error.message}` }); @@ -501,7 +522,7 @@ function registerApp(app, options = {}) { const org = config?.organization || owner; const analysisReport = await generateOrganizationAnalysisReport(context.octokit, org); - const reportIssue = await context.octokit.issues.create({ + const reportIssue = await issueOps.createIssue(context.octokit,{ owner, repo, title: `Organization Analysis Report - ${new Date().toISOString().split('T')[0]}`, @@ -509,7 +530,7 @@ function registerApp(app, options = {}) { labels: ['analysis', 'report', 'automation'] }); - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -531,7 +552,7 @@ function registerApp(app, options = {}) { issueNumber ); if (strategyCheck.error) { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -560,7 +581,7 @@ function registerApp(app, options = {}) { response += `💡 **Recommendation:** Current merge strategy is appropriate.`; } - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -577,7 +598,7 @@ function registerApp(app, options = {}) { const allowedAdmins = config.signed_commit_strategy?.admin_users || []; if (allowedAdmins.length > 0 && !allowedAdmins.includes(sender.login)) { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -595,7 +616,7 @@ function registerApp(app, options = {}) { ); if (result.success) { if (result.action === 'temporarily_allowed_merge_commits') { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -605,7 +626,7 @@ function registerApp(app, options = {}) { `⚠️ Remember: Merge settings will restore to rebase-only after 1 hour.` }); } else { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -613,7 +634,7 @@ function registerApp(app, options = {}) { }); } } else { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -631,7 +652,7 @@ function registerApp(app, options = {}) { // Verify the comment is on a PR (not a plain issue) if (!context.payload.issue.pull_request) { - await context.octokit.issues.createComment({ + await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -641,7 +662,7 @@ function registerApp(app, options = {}) { } // Post a "working on it" indicator - const workingComment = await context.octokit.issues.createComment({ + const workingComment = await issueOps.createComment(context.octokit,{ owner, repo, issue_number: issueNumber, @@ -651,7 +672,7 @@ function registerApp(app, options = {}) { const result = await reviewPullRequest(context.octokit, owner, repo, issueNumber); if (!result.success) { - await context.octokit.issues.updateComment({ + await issueOps.updateComment(context.octokit,{ owner, repo, comment_id: workingComment.data.id, @@ -659,7 +680,7 @@ function registerApp(app, options = {}) { }); } else { // Delete the "working" comment since the review was posted separately - await context.octokit.issues.deleteComment({ + await issueOps.deleteComment(context.octokit,{ owner, repo, comment_id: workingComment.data.id