From 6b0b3729f3c0eb218a25904e14afc5a85c254677 Mon Sep 17 00:00:00 2001 From: kimi-reviewer-andresmgsl Date: Wed, 22 Jul 2026 21:32:06 +0000 Subject: [PATCH] Fix review event mapping and body validation - Map CLI 'approve' to Forgejo's expected 'APPROVED' event. - Require a non-empty body for request-changes and comment events. - Update API test expectation and add CLI tests for body validation. --- src/cli.js | 8 ++++++-- test/api.test.js | 4 ++-- test/cli.test.js | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/cli.js b/src/cli.js index bfd9a2b..350db6a 100755 --- a/src/cli.js +++ b/src/cli.js @@ -747,7 +747,7 @@ pr const config = loadConfig(); const client = ForgejoClient.fromConfig(config); const eventMap = { - approve: 'APPROVE', + approve: 'APPROVED', 'request-changes': 'REQUEST_CHANGES', comment: 'COMMENT', }; @@ -756,7 +756,11 @@ pr console.error(`Invalid review event: ${options.event}. Must be approve, request-changes, or comment.`); process.exit(1); } - const body = readBodyOption(options) || ''; + const body = (readBodyOption(options) || '').trim(); + if (event !== 'APPROVED' && body.length === 0) { + console.error(`Review event ${options.event} requires a non-empty body. Use -b/--body or --body-file.`); + process.exit(1); + } await client.createPullRequestReview(options.owner, options.repo, options.number, event, body); console.log(`Review submitted on !${options.number}: ${event}.`); } catch (err) { diff --git a/test/api.test.js b/test/api.test.js index 8eb0656..317ac3e 100644 --- a/test/api.test.js +++ b/test/api.test.js @@ -163,10 +163,10 @@ test('createPullRequestComment posts to the issue comments endpoint', async () = test('createPullRequestReview posts the review event and body', async () => { const calls = mockFetch(() => jsonResponse({ id: 88 })); const client = new ForgejoClient('https://forge.test', 'tok'); - await client.createPullRequestReview('owner', 'repo', 7, 'APPROVE', 'Ship it.'); + await client.createPullRequestReview('owner', 'repo', 7, 'APPROVED', 'Ship it.'); assert.equal(calls[0].url, 'https://forge.test/api/v1/repos/owner/repo/pulls/7/reviews'); assert.equal(calls[0].opts.method, 'POST'); const body = JSON.parse(calls[0].opts.body); - assert.equal(body.event, 'APPROVE'); + assert.equal(body.event, 'APPROVED'); assert.equal(body.body, 'Ship it.'); }); diff --git a/test/cli.test.js b/test/cli.test.js index 77f9826..28a3aff 100644 --- a/test/cli.test.js +++ b/test/cli.test.js @@ -111,3 +111,40 @@ test('pr review rejects an invalid event before any network call', () => { fs.unlinkSync(cfg); } }); + +test('pr review request-changes rejects a missing body before any network call', () => { + const cfg = path.join(os.tmpdir(), `stoke-cfg-${process.pid}.json`); + fs.writeFileSync(cfg, JSON.stringify({ url: 'https://forge.test', token: 'tok' })); + try { + const res = run(['pr', 'review', '-o', 'o', '-r', 'r', '-n', '1', '--event', 'request-changes'], { STOKE_CONFIG_FILE: cfg }); + assert.equal(res.status, 1); + assert.match(res.stderr, /requires a non-empty body/); + } finally { + fs.unlinkSync(cfg); + } +}); + +test('pr review comment rejects a whitespace-only body before any network call', () => { + const cfg = path.join(os.tmpdir(), `stoke-cfg-${process.pid}.json`); + fs.writeFileSync(cfg, JSON.stringify({ url: 'https://forge.test', token: 'tok' })); + try { + const res = run(['pr', 'review', '-o', 'o', '-r', 'r', '-n', '1', '--event', 'comment', '-b', ' '], { STOKE_CONFIG_FILE: cfg }); + assert.equal(res.status, 1); + assert.match(res.stderr, /requires a non-empty body/); + } finally { + fs.unlinkSync(cfg); + } +}); + +test('pr review approve allows an empty body before any network call', () => { + const cfg = path.join(os.tmpdir(), `stoke-cfg-${process.pid}.json`); + fs.writeFileSync(cfg, JSON.stringify({ url: 'https://forge.test', token: 'tok' })); + try { + const res = run(['pr', 'review', '-o', 'o', '-r', 'r', '-n', '1', '--event', 'approve'], { STOKE_CONFIG_FILE: cfg }); + // It fails at the network call, not at validation. + assert.equal(res.status, 1); + assert.doesNotMatch(res.stderr, /requires a non-empty body/); + } finally { + fs.unlinkSync(cfg); + } +});