test(review): prove fail-open status contract
This commit is contained in:
@@ -44,7 +44,7 @@ Dependencies: **none**. Node ≥ 22.13 (uses `node:sqlite`, global `fetch`).
|
|||||||
|
|
||||||
- Commit status context: `granthi-review`. verdict pass → `success`; warn → `success` with warning description; fail → `failure`.
|
- Commit status context: `granthi-review`. verdict pass → `success`; warn → `success` with warning description; fail → `failure`.
|
||||||
- Branch protection on pilot repos requires the `granthi-review` context but does **not** apply to admins — an admin merge is the override. The blocking comment says so explicitly.
|
- Branch protection on pilot repos requires the `granthi-review` context but does **not** apply to admins — an admin merge is the override. The blocking comment says so explicitly.
|
||||||
- **Fail-open with visibility**: if the router/LLM is down or output is unparseable after retry, the service posts status `warning` with "review unavailable" — it never blocks silently and never fakes a pass.
|
- **Fail-open with visibility**: if the router/LLM is down or output is unparseable after retry, the service posts status `success` with "review unavailable" so the required check does not block. The error remains visible in the description and review ledger.
|
||||||
- Bare pushes (no PR): commit status + ledger + dashboard only. Gitea 1.27 has no commit-comment API endpoint, so no comment is posted for pushes.
|
- Bare pushes (no PR): commit status + ledger + dashboard only. Gitea 1.27 has no commit-comment API endpoint, so no comment is posted for pushes.
|
||||||
|
|
||||||
## Config
|
## Config
|
||||||
@@ -96,7 +96,7 @@ and retries the post once. Without `admin_token` the 403 surfaces as before.
|
|||||||
|
|
||||||
- **Full push-range review**: pushes are reviewed as ONE compare diff `before...after` (web compare `.diff` route — the 1.27 API has no compare diff variant). New-branch pushes (`before` = zero sha) fall back to the head commit diff. No more first-20-commits slicing under a head-sha status.
|
- **Full push-range review**: pushes are reviewed as ONE compare diff `before...after` (web compare `.diff` route — the 1.27 API has no compare diff variant). New-branch pushes (`before` = zero sha) fall back to the head commit diff. No more first-20-commits slicing under a head-sha status.
|
||||||
- **Truncated diff ≠ pass**: when the 60KB cap truncates the diff, the prompt and the status description both say `partial review (diff truncated)` and the verdict is capped at `warn` (a confirmed-critical `fail` still fails).
|
- **Truncated diff ≠ pass**: when the 60KB cap truncates the diff, the prompt and the status description both say `partial review (diff truncated)` and the verdict is capped at `warn` (a confirmed-critical `fail` still fails).
|
||||||
- **Diff fetch failure ≠ pass**: any diff-fetch failure (throw, non-200, empty body for a non-empty push/PR) short-circuits to status `warning` — `review unavailable — diff fetch failed (not blocking)` — with a ledger row. A diffless prompt is never sent.
|
- **Diff fetch failure ≠ review**: any diff-fetch failure (throw, non-200, empty body for a non-empty push/PR) short-circuits to status `success` — `review unavailable — diff fetch failed (not blocking)` — with the error preserved in a ledger row. A diffless prompt is never sent.
|
||||||
- **Crash-safe queue**: every accepted job is persisted to sqlite (`queue_jobs`) before the pending status posts; on startup, rows that never reached a final state are re-enqueued, so a restart no longer strands a sha at `pending`.
|
- **Crash-safe queue**: every accepted job is persisted to sqlite (`queue_jobs`) before the pending status posts; on startup, rows that never reached a final state are re-enqueued, so a restart no longer strands a sha at `pending`.
|
||||||
- **403 self-heal**: see `admin_token` above. `scripts/wire-repos.mjs` wires the webhook + bot collaborator across ALL repos on the peer (idempotent; re-runnable).
|
- **403 self-heal**: see `admin_token` above. `scripts/wire-repos.mjs` wires the webhook + bot collaborator across ALL repos on the peer (idempotent; re-runnable).
|
||||||
|
|
||||||
|
|||||||
+2
-2
@@ -67,8 +67,8 @@ export async function runReview({ cfg, gitea, adminGitea, db, job, withGlobal, l
|
|||||||
}));
|
}));
|
||||||
|
|
||||||
// 1. diff — any fetch failure (throw, non-200, or empty body for a job that
|
// 1. diff — any fetch failure (throw, non-200, or empty body for a job that
|
||||||
// has commits) short-circuits to a 'warning' status. A diffless prompt would
|
// has commits) short-circuits to a non-blocking 'success' status. A diffless
|
||||||
// make the model "review" nothing and pass; never send one.
|
// prompt would make the model "review" nothing and pass; never send one.
|
||||||
let diff = '', truncated = false, diffBytes = 0, partialDiff = false, partialNote = null;
|
let diff = '', truncated = false, diffBytes = 0, partialDiff = false, partialNote = null;
|
||||||
try {
|
try {
|
||||||
const fetched = await fetchDiff({
|
const fetched = await fetchDiff({
|
||||||
|
|||||||
@@ -0,0 +1,45 @@
|
|||||||
|
import { test } from 'node:test';
|
||||||
|
import assert from 'node:assert/strict';
|
||||||
|
import { openDb } from '../src/lib/db.js';
|
||||||
|
import { runReview } from '../src/lib/review.js';
|
||||||
|
|
||||||
|
const HEAD = 'a'.repeat(40);
|
||||||
|
|
||||||
|
test('an unavailable review posts success without hiding the failure', async () => {
|
||||||
|
const statuses = [];
|
||||||
|
const db = openDb(':memory:');
|
||||||
|
const gitea = {
|
||||||
|
postStatus: async (_owner, _repo, _sha, body) => statuses.push(body),
|
||||||
|
getCommitDiff: async () => { throw new Error('review service unavailable'); }
|
||||||
|
};
|
||||||
|
const job = {
|
||||||
|
owner: 'o', repo: 'r', sha: HEAD, before: null, prIndex: null,
|
||||||
|
event: 'push', pusher: 'tester', authors: [], commitMessages: []
|
||||||
|
};
|
||||||
|
|
||||||
|
const result = await runReview({
|
||||||
|
cfg: {
|
||||||
|
publicBaseUrl: 'https://reviews.example.test',
|
||||||
|
maxDiffBytes: 1024,
|
||||||
|
model: 'claude-cli/claude-sonnet-4-6',
|
||||||
|
botUsername: 'reviewer'
|
||||||
|
},
|
||||||
|
gitea,
|
||||||
|
adminGitea: null,
|
||||||
|
db,
|
||||||
|
job,
|
||||||
|
withGlobal: async (fn) => fn(),
|
||||||
|
log: () => {}
|
||||||
|
});
|
||||||
|
|
||||||
|
assert.equal(result.ok, false);
|
||||||
|
assert.deepEqual(statuses.map(({ state }) => state), ['pending', 'success']);
|
||||||
|
assert.equal(statuses[1].context, 'granthi-review');
|
||||||
|
assert.equal(statuses[1].description, 'review unavailable — diff fetch failed (not blocking)');
|
||||||
|
|
||||||
|
const persisted = db.prepare(
|
||||||
|
'SELECT status_state, error FROM reviews WHERE repo = ? AND sha = ?'
|
||||||
|
).get('o/r', HEAD);
|
||||||
|
assert.equal(persisted.status_state, 'success');
|
||||||
|
assert.match(persisted.error, /review service unavailable/);
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user