Compare commits
17
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
1144864b06 | ||
|
|
2099a16a1f | ||
|
|
a12f94f459 | ||
|
|
0db44a4d73 | ||
|
|
6b131e1427 | ||
|
|
a76221578c | ||
|
|
0dec11b5ed | ||
|
|
6478ce2bfa | ||
|
|
c648a5a6da | ||
|
|
10bbd598f3 | ||
|
|
ad8c1256d8 | ||
|
|
a531323865 | ||
|
|
d0b21927e7 | ||
|
|
e32c600215 | ||
|
|
f35a75f787 | ||
|
|
9f2927c33e | ||
|
|
aa6f2c8715 |
@@ -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
|
||||||
@@ -60,18 +60,45 @@ Dependencies: **none**. Node ≥ 22.13 (uses `node:sqlite`, global `fetch`).
|
|||||||
"routerUrl": "http://localhost:5497",
|
"routerUrl": "http://localhost:5497",
|
||||||
"model": "anthropic/claude-sonnet-4-6",
|
"model": "anthropic/claude-sonnet-4-6",
|
||||||
"tenantId": "nirlab",
|
"tenantId": "nirlab",
|
||||||
"publicBaseUrl": "http://localhost:5498"
|
"publicBaseUrl": "http://localhost:5498",
|
||||||
|
"admin_token": "<admin-scoped token (user nirpa) — used ONLY to self-heal bot access>",
|
||||||
|
"botUsername": "shre-reviewer"
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|
||||||
Bot identity: Gitea user `shre-reviewer` (created via `gitea admin user create`).
|
Bot identity: Gitea user `shre-reviewer` (created via `gitea admin user create`).
|
||||||
|
|
||||||
|
`admin_token` powers the 403 self-heal: when a status/comment post returns 403
|
||||||
|
(a repo auto-wired by the admin default hook where the bot has no access yet),
|
||||||
|
the service adds `shre-reviewer` as collaborator (write) via the admin token
|
||||||
|
and retries the post once. Without `admin_token` the 403 surfaces as before.
|
||||||
|
|
||||||
## Deploy (Mac dev tier)
|
## Deploy (Mac dev tier)
|
||||||
|
|
||||||
- `launchd/ai.granthi.review.plist` → `~/Library/LaunchAgents/`, KeepAlive service on :5498.
|
- `launchd/ai.granthi.review.plist` → `~/Library/LaunchAgents/`, KeepAlive service on :5498, running from the `~/Services/granthi-review` clone.
|
||||||
- `launchd/ai.granthi.review.digest.plist` → nightly 03:30 `--digest` run.
|
- `launchd/ai.granthi.review.digest.plist` → nightly 03:30 `--digest` run.
|
||||||
- Webhooks: Gitea **system webhook** (admin → hooks) pointed at `http://host.docker.internal:5498/webhook` (the gitea container reaches the host that way), events push + pull_request, secret = `webhookSecret`. Fallback: per-repo hooks on pilot repos.
|
- Webhooks (all target `http://host.docker.internal:5498/webhook`, events push + pull_request, secret = `webhookSecret`):
|
||||||
- Pilot gating: `nirpa/gitea-distro` main branch protection lists `granthi-review` in `status_check_contexts` (alongside the CI build context); `enable_approvals_whitelist`/admin-bypass left so admins can merge over a block.
|
- **Per-repo hooks** on the pilot repos: `nirpa/gitea-distro`, `nirpa/granthi-staging-infra`, `nirpa/granthi-review`.
|
||||||
|
- **Admin default hook** (`POST /api/v1/admin/hooks`) — Gitea 1.27's API can only create *default* hooks (copied into repos created after the hook exists), not *system* hooks, so every **new** repo is auto-wired but pre-existing repos need a per-repo hook. To cover the whole estate in one shot, create the same hook as a **system webhook** via the admin web UI (Site Administration → Integrations → Webhooks → System Webhooks) — API tokens cannot do it in 1.27.
|
||||||
|
- Pilot gating: `nirpa/gitea-distro` main branch protection lists `granthi-review` in `status_check_contexts` (alongside `build-branded-image / build`); `block_admin_merge_override` is false, so admins can merge over a block (the override path).
|
||||||
|
|
||||||
|
### Router integration notes (learned live)
|
||||||
|
|
||||||
|
- The shre-router request MUST carry `tools:false` and `raw:true`, otherwise keyword fast-paths (current-info briefing, store-resolver, retail dispatch) hijack review prompts whose diff content mentions github/sales/store-ish words and return non-review text.
|
||||||
|
- The domain preflight can 400 (`agent_mismatch`) when the intent classifier misreads a diff; attempt 2 retries as the c-suite `fallbackAgentId` (default `architect`), which bypasses the tier-gated preflight.
|
||||||
|
- The router may serve a different model than requested (`_shre.model` is recorded in the ledger's `model` column).
|
||||||
|
|
||||||
|
### Known v1 limitations
|
||||||
|
|
||||||
|
- Bare pushes get status + ledger only (no Gitea commit-comment API).
|
||||||
|
|
||||||
|
### Hardening (fix/review-hardening)
|
||||||
|
|
||||||
|
- **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).
|
||||||
|
- **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`.
|
||||||
|
- **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).
|
||||||
|
|
||||||
## Adding the Reviews tab later (Granthi overlay)
|
## Adding the Reviews tab later (Granthi overlay)
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,95 @@
|
|||||||
|
#!/usr/bin/env node
|
||||||
|
// Wire EVERY repo on the peer forge for granthi-review:
|
||||||
|
// - create the push+pull_request webhook -> <hookUrl> (HMAC secret from the
|
||||||
|
// service config) on every repo that lacks it (idempotent: skipped when a
|
||||||
|
// hook with that exact URL already exists)
|
||||||
|
// - add the bot (shre-reviewer) as collaborator (write) so status/comment
|
||||||
|
// posts don't 403
|
||||||
|
// Re-runnable at any time. Uses the admin-scoped token (config admin_token).
|
||||||
|
//
|
||||||
|
// Usage: node scripts/wire-repos.mjs [--dry-run]
|
||||||
|
|
||||||
|
import { readFileSync } from 'node:fs';
|
||||||
|
import { homedir } from 'node:os';
|
||||||
|
import { join } from 'node:path';
|
||||||
|
|
||||||
|
const CONFIG_DIR = process.env.GRANTHI_REVIEW_HOME || join(homedir(), '.granthi-review');
|
||||||
|
const cfg = JSON.parse(readFileSync(join(CONFIG_DIR, 'config.json'), 'utf8'));
|
||||||
|
|
||||||
|
const base = (cfg.forgeBaseUrl || 'http://localhost:3030').replace(/\/$/, '');
|
||||||
|
const adminToken = cfg.admin_token || process.env.GITEA_CLAUDE_SIA_TOKEN;
|
||||||
|
const secret = cfg.webhookSecret;
|
||||||
|
const botUsername = cfg.botUsername || 'shre-reviewer';
|
||||||
|
const hookUrl = process.env.GRANTHI_REVIEW_HOOK_URL || 'http://host.docker.internal:5498/webhook';
|
||||||
|
const dryRun = process.argv.includes('--dry-run');
|
||||||
|
|
||||||
|
if (!adminToken) { console.error('no admin token (config admin_token or $GITEA_CLAUDE_SIA_TOKEN)'); process.exit(1); }
|
||||||
|
if (!secret) { console.error('no webhookSecret in config'); process.exit(1); }
|
||||||
|
|
||||||
|
async function api(method, path, body, ok = [200, 201, 204]) {
|
||||||
|
const res = await fetch(`${base}${path}`, {
|
||||||
|
method,
|
||||||
|
headers: {
|
||||||
|
Authorization: `token ${adminToken}`,
|
||||||
|
...(body ? { 'Content-Type': 'application/json' } : {})
|
||||||
|
},
|
||||||
|
body: body ? JSON.stringify(body) : undefined
|
||||||
|
});
|
||||||
|
if (!ok.includes(res.status)) {
|
||||||
|
const text = await res.text().catch(() => '');
|
||||||
|
throw new Error(`${method} ${path} -> ${res.status}: ${text.slice(0, 200)}`);
|
||||||
|
}
|
||||||
|
if (res.status === 204) return null;
|
||||||
|
return res.json().catch(() => null);
|
||||||
|
}
|
||||||
|
|
||||||
|
async function allRepos() {
|
||||||
|
const repos = [];
|
||||||
|
for (let page = 1; ; page++) {
|
||||||
|
const out = await api('GET', `/api/v1/repos/search?limit=50&page=${page}`);
|
||||||
|
const batch = out?.data || [];
|
||||||
|
repos.push(...batch);
|
||||||
|
if (batch.length < 50) break;
|
||||||
|
}
|
||||||
|
return repos;
|
||||||
|
}
|
||||||
|
|
||||||
|
const repos = await allRepos();
|
||||||
|
let hooksCreated = 0, hooksPresent = 0, collabAdded = 0, skipped = 0, errors = 0;
|
||||||
|
|
||||||
|
for (const r of repos) {
|
||||||
|
const full = r.full_name;
|
||||||
|
const [owner, name] = full.split('/');
|
||||||
|
try {
|
||||||
|
if (r.archived || r.empty || r.mirror) { skipped++; console.log(`skip ${full} (${r.archived ? 'archived' : r.mirror ? 'mirror' : 'empty'})`); continue; }
|
||||||
|
const hooks = await api('GET', `/api/v1/repos/${full}/hooks?limit=50`) || [];
|
||||||
|
const has = hooks.some(h => h?.config?.url === hookUrl);
|
||||||
|
if (has) {
|
||||||
|
hooksPresent++;
|
||||||
|
} else if (dryRun) {
|
||||||
|
console.log(`would wire ${full}`);
|
||||||
|
} else {
|
||||||
|
await api('POST', `/api/v1/repos/${full}/hooks`, {
|
||||||
|
type: 'gitea',
|
||||||
|
active: true,
|
||||||
|
events: ['push', 'pull_request'],
|
||||||
|
config: { url: hookUrl, content_type: 'json', secret }
|
||||||
|
});
|
||||||
|
hooksCreated++;
|
||||||
|
console.log(`wired ${full}`);
|
||||||
|
}
|
||||||
|
// collaborator write for the bot (idempotent PUT); skip bot-owned repos
|
||||||
|
if (owner !== botUsername && !dryRun) {
|
||||||
|
await api('PUT', `/api/v1/repos/${full}/collaborators/${botUsername}`, { permission: 'write' });
|
||||||
|
collabAdded++;
|
||||||
|
}
|
||||||
|
} catch (e) {
|
||||||
|
errors++;
|
||||||
|
console.error(`ERROR ${full}: ${e.message}`);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
console.log(`\ntotal repos: ${repos.length}`);
|
||||||
|
console.log(`hooks created: ${hooksCreated}, already wired: ${hooksPresent}, skipped (archived/empty/mirror): ${skipped}`);
|
||||||
|
console.log(`collaborator (write) ensured: ${collabAdded}, errors: ${errors}`);
|
||||||
|
process.exit(errors ? 2 : 0);
|
||||||
+5
-1
@@ -19,7 +19,8 @@ const DEFAULTS = {
|
|||||||
digestDir: join(CONFIG_DIR, 'digests'),
|
digestDir: join(CONFIG_DIR, 'digests'),
|
||||||
maxDiffBytes: 60 * 1024,
|
maxDiffBytes: 60 * 1024,
|
||||||
routerTimeoutMs: 240000,
|
routerTimeoutMs: 240000,
|
||||||
digestLastN: 20
|
digestLastN: 20,
|
||||||
|
botUsername: 'shre-reviewer'
|
||||||
};
|
};
|
||||||
|
|
||||||
export function loadConfig() {
|
export function loadConfig() {
|
||||||
@@ -28,6 +29,9 @@ export function loadConfig() {
|
|||||||
fileCfg = JSON.parse(readFileSync(join(CONFIG_DIR, 'config.json'), 'utf8'));
|
fileCfg = JSON.parse(readFileSync(join(CONFIG_DIR, 'config.json'), 'utf8'));
|
||||||
} catch { /* config file optional; env/defaults apply */ }
|
} catch { /* config file optional; env/defaults apply */ }
|
||||||
const cfg = { ...DEFAULTS, ...fileCfg };
|
const cfg = { ...DEFAULTS, ...fileCfg };
|
||||||
|
// admin-scoped token (config key: admin_token) — used ONLY to self-heal bot
|
||||||
|
// permissions (add the bot as collaborator when a status/comment post 403s).
|
||||||
|
cfg.adminToken = fileCfg.admin_token || '';
|
||||||
// env overrides
|
// env overrides
|
||||||
if (process.env.GRANTHI_REVIEW_PORT) cfg.port = Number(process.env.GRANTHI_REVIEW_PORT);
|
if (process.env.GRANTHI_REVIEW_PORT) cfg.port = Number(process.env.GRANTHI_REVIEW_PORT);
|
||||||
if (process.env.GRANTHI_REVIEW_TOKEN) cfg.botToken = process.env.GRANTHI_REVIEW_TOKEN;
|
if (process.env.GRANTHI_REVIEW_TOKEN) cfg.botToken = process.env.GRANTHI_REVIEW_TOKEN;
|
||||||
|
|||||||
@@ -0,0 +1,19 @@
|
|||||||
|
// Self-healing bot access: new repos wired by the admin default hook 403 the
|
||||||
|
// bot's status/comment posts because shre-reviewer is not a collaborator yet.
|
||||||
|
// On a 403 we add the bot as collaborator (write) using the admin-scoped
|
||||||
|
// token, then retry the original call exactly once.
|
||||||
|
|
||||||
|
export async function withCollabRetry(fn, { adminGitea, owner, repo, botUsername, log }) {
|
||||||
|
try {
|
||||||
|
return await fn();
|
||||||
|
} catch (e) {
|
||||||
|
if (e?.status !== 403) throw e;
|
||||||
|
if (!adminGitea) {
|
||||||
|
log?.(`[${owner}/${repo}] 403 from forge and no admin_token configured — cannot self-heal collaborator access`);
|
||||||
|
throw e;
|
||||||
|
}
|
||||||
|
log?.(`[${owner}/${repo}] 403 from forge — adding ${botUsername} as collaborator (write) and retrying once`);
|
||||||
|
await adminGitea.addCollaborator(owner, repo, botUsername, 'write');
|
||||||
|
return fn();
|
||||||
|
}
|
||||||
|
}
|
||||||
+59
-1
@@ -26,16 +26,36 @@ const DDL = [
|
|||||||
ts TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now'))
|
ts TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now'))
|
||||||
)`,
|
)`,
|
||||||
`CREATE INDEX IF NOT EXISTS idx_reviews_repo_ts ON reviews(repo, ts DESC)`,
|
`CREATE INDEX IF NOT EXISTS idx_reviews_repo_ts ON reviews(repo, ts DESC)`,
|
||||||
`CREATE INDEX IF NOT EXISTS idx_reviews_sha ON reviews(sha)`
|
`CREATE INDEX IF NOT EXISTS idx_reviews_sha ON reviews(sha)`,
|
||||||
|
`CREATE TABLE IF NOT EXISTS queue_jobs (
|
||||||
|
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||||
|
repo TEXT NOT NULL,
|
||||||
|
sha TEXT NOT NULL,
|
||||||
|
job TEXT NOT NULL,
|
||||||
|
state TEXT NOT NULL DEFAULT 'queued',
|
||||||
|
outcome TEXT,
|
||||||
|
created_ts TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')),
|
||||||
|
started_ts TEXT,
|
||||||
|
finished_ts TEXT
|
||||||
|
)`,
|
||||||
|
`CREATE INDEX IF NOT EXISTS idx_queue_jobs_state ON queue_jobs(state)`
|
||||||
];
|
];
|
||||||
|
|
||||||
export function openDb(dbPath) {
|
export function openDb(dbPath) {
|
||||||
mkdirSync(dirname(dbPath), { recursive: true });
|
mkdirSync(dirname(dbPath), { recursive: true });
|
||||||
const db = new DatabaseSync(dbPath);
|
const db = new DatabaseSync(dbPath);
|
||||||
for (const stmt of DDL) db.prepare(stmt).run();
|
for (const stmt of DDL) db.prepare(stmt).run();
|
||||||
|
ensureColumn(db, 'queue_jobs', 'started_ts', 'TEXT');
|
||||||
return db;
|
return db;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function ensureColumn(db, table, column, type) {
|
||||||
|
const columns = db.prepare(`PRAGMA table_info(${table})`).all();
|
||||||
|
if (!columns.some((c) => c.name === column)) {
|
||||||
|
db.prepare(`ALTER TABLE ${table} ADD COLUMN ${column} ${type}`).run();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
export function insertReview(db, r) {
|
export function insertReview(db, r) {
|
||||||
const stmt = db.prepare(`
|
const stmt = db.prepare(`
|
||||||
INSERT INTO reviews (repo, sha, pr, event, pusher, authors, trailers, axes,
|
INSERT INTO reviews (repo, sha, pr, event, pusher, authors, trailers, axes,
|
||||||
@@ -55,6 +75,44 @@ export function insertReview(db, r) {
|
|||||||
return res.lastInsertRowid;
|
return res.lastInsertRowid;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// --- persistent work queue (crash-safe: rows written before the pending
|
||||||
|
// status posts; rows that never reach 'done' are re-enqueued on startup) ---
|
||||||
|
|
||||||
|
export function enqueueJob(db, job) {
|
||||||
|
const res = db.prepare(
|
||||||
|
`INSERT INTO queue_jobs (repo, sha, job) VALUES (?, ?, ?)`
|
||||||
|
).run(`${job.owner}/${job.repo}`, String(job.sha), JSON.stringify(job));
|
||||||
|
return res.lastInsertRowid;
|
||||||
|
}
|
||||||
|
|
||||||
|
export function startJob(db, id) {
|
||||||
|
db.prepare(
|
||||||
|
`UPDATE queue_jobs SET state = 'running',
|
||||||
|
started_ts = strftime('%Y-%m-%dT%H:%M:%fZ','now') WHERE id = ?`
|
||||||
|
).run(id);
|
||||||
|
}
|
||||||
|
|
||||||
|
export function finishJob(db, id, outcome = null) {
|
||||||
|
db.prepare(
|
||||||
|
`UPDATE queue_jobs SET state = 'done', outcome = ?,
|
||||||
|
finished_ts = strftime('%Y-%m-%dT%H:%M:%fZ','now') WHERE id = ?`
|
||||||
|
).run(outcome, id);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Startup reconciliation: rows still 'queued' belonged to a previous process
|
||||||
|
// that died mid-review (their posted status is stuck at 'pending').
|
||||||
|
export function recoverPendingJobs(db) {
|
||||||
|
const rows = db.prepare(
|
||||||
|
`SELECT id, job FROM queue_jobs WHERE state != 'done' ORDER BY id ASC`
|
||||||
|
).all();
|
||||||
|
const out = [];
|
||||||
|
for (const row of rows) {
|
||||||
|
try { out.push({ id: row.id, job: JSON.parse(row.job) }); }
|
||||||
|
catch { finishJob(db, row.id, 'unparseable-job'); }
|
||||||
|
}
|
||||||
|
return out;
|
||||||
|
}
|
||||||
|
|
||||||
export function listReviews(db, repo, limit = 50) {
|
export function listReviews(db, repo, limit = 50) {
|
||||||
return db.prepare(
|
return db.prepare(
|
||||||
'SELECT * FROM reviews WHERE repo = ? ORDER BY id DESC LIMIT ?'
|
'SELECT * FROM reviews WHERE repo = ? ORDER BY id DESC LIMIT ?'
|
||||||
|
|||||||
+29
-3
@@ -1,5 +1,9 @@
|
|||||||
// Minimal Gitea API client (fetch-based, no deps).
|
// Minimal Gitea API client (fetch-based, no deps).
|
||||||
|
|
||||||
|
// URL path segment from webhook/API-supplied values (owner, repo, refs,
|
||||||
|
// shas): never trust them as URL-safe — branch names can carry '/', '#', '?'.
|
||||||
|
const seg = (s) => encodeURIComponent(String(s));
|
||||||
|
|
||||||
export class GiteaClient {
|
export class GiteaClient {
|
||||||
constructor({ baseUrl, token, timeoutMs = 30000 }) {
|
constructor({ baseUrl, token, timeoutMs = 30000 }) {
|
||||||
this.baseUrl = baseUrl.replace(/\/$/, '');
|
this.baseUrl = baseUrl.replace(/\/$/, '');
|
||||||
@@ -28,17 +32,39 @@ export class GiteaClient {
|
|||||||
|
|
||||||
// PR diff
|
// PR diff
|
||||||
getPrDiff(owner, repo, index) {
|
getPrDiff(owner, repo, index) {
|
||||||
return this.req('GET', `/api/v1/repos/${owner}/${repo}/pulls/${index}.diff`, { raw: true });
|
return this.req('GET', `/api/v1/repos/${seg(owner)}/${seg(repo)}/pulls/${seg(index)}.diff`, { raw: true });
|
||||||
}
|
}
|
||||||
|
|
||||||
// Single-commit diff
|
// Single-commit diff
|
||||||
getCommitDiff(owner, repo, sha) {
|
getCommitDiff(owner, repo, sha) {
|
||||||
return this.req('GET', `/api/v1/repos/${owner}/${repo}/git/commits/${sha}.diff`, { raw: true });
|
return this.req('GET', `/api/v1/repos/${seg(owner)}/${seg(repo)}/git/commits/${seg(sha)}.diff`, { raw: true });
|
||||||
}
|
}
|
||||||
|
|
||||||
// Compare (commit list between two shas)
|
// Compare (commit list between two shas)
|
||||||
compare(owner, repo, before, after) {
|
compare(owner, repo, before, after) {
|
||||||
return this.req('GET', `/api/v1/repos/${owner}/${repo}/compare/${before}...${after}`);
|
return this.req('GET', `/api/v1/repos/${seg(owner)}/${seg(repo)}/compare/${seg(before)}...${seg(after)}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Full-range diff for a push (base...head) in ONE request. Gitea 1.27's API
|
||||||
|
// has no compare .diff variant, but the web compare route serves .diff.
|
||||||
|
// CAVEAT (probed live on 1.27.1): the web route does NOT honor
|
||||||
|
// `Authorization: token` for private repos — it resolves as anonymous and
|
||||||
|
// answers 404 even for valid refs. Callers must be ready to fall back to
|
||||||
|
// the API-based sources below (fetchDiff in review.js does).
|
||||||
|
getCompareDiff(owner, repo, before, after) {
|
||||||
|
return this.req('GET', `/${seg(owner)}/${seg(repo)}/compare/${seg(before)}...${seg(after)}.diff`, { raw: true });
|
||||||
|
}
|
||||||
|
|
||||||
|
// Repo metadata (used for default_branch when the job doesn't carry it)
|
||||||
|
getRepo(owner, repo) {
|
||||||
|
return this.req('GET', `/api/v1/repos/${seg(owner)}/${seg(repo)}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Add a collaborator (requires owner/admin token). 204 on success.
|
||||||
|
addCollaborator(owner, repo, username, permission = 'write') {
|
||||||
|
return this.req('PUT', `/api/v1/repos/${owner}/${repo}/collaborators/${username}`, {
|
||||||
|
body: { permission }, ok: [204]
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
// File content at HEAD (returns null when absent)
|
// File content at HEAD (returns null when absent)
|
||||||
|
|||||||
+3
-1
@@ -19,12 +19,14 @@ Output schema (every key required):
|
|||||||
"findings": [
|
"findings": [
|
||||||
{"title": "...", "file": "path", "line": 123, "severity": "critical"|"high"|"medium"|"low",
|
{"title": "...", "file": "path", "line": 123, "severity": "critical"|"high"|"medium"|"low",
|
||||||
"confidence": "confirmed"|"plausible", "axis": "correctness"|"security"|"tests_coverage"|"design_simplicity"|"performance"|"style"|"breaking_changes"|"docs",
|
"confidence": "confirmed"|"plausible", "axis": "correctness"|"security"|"tests_coverage"|"design_simplicity"|"performance"|"style"|"breaking_changes"|"docs",
|
||||||
|
"resolved_by_this_change": true|false,
|
||||||
"detail": "what is wrong, concrete failure scenario, and the fix"}
|
"detail": "what is wrong, concrete failure scenario, and the fix"}
|
||||||
],
|
],
|
||||||
"verdict": "pass"|"warn"|"fail"
|
"verdict": "pass"|"warn"|"fail"
|
||||||
}
|
}
|
||||||
|
|
||||||
Rules:
|
Rules:
|
||||||
|
- A finding must describe a defect that EXISTS IN THE RESULTING CODE after this diff is applied. The pre-existing bug that this diff FIXES is not a finding — acknowledge it in the correctness rationale instead. If you still list it (e.g. for the record), you MUST set "resolved_by_this_change": true on that finding; findings the diff introduces or leaves unfixed get "resolved_by_this_change": false.
|
||||||
- "confirmed" means you can trace a concrete failure from the diff alone; anything needing unseen context is "plausible".
|
- "confirmed" means you can trace a concrete failure from the diff alone; anything needing unseen context is "plausible".
|
||||||
- severity critical/high is reserved for real correctness bugs, security holes, data loss, or breaking API changes.
|
- severity critical/high is reserved for real correctness bugs, security holes, data loss, or breaking API changes.
|
||||||
- verdict "fail" ONLY when a confirmed critical/high correctness or security finding exists; "warn" for notable but non-blocking issues; otherwise "pass".
|
- verdict "fail" ONLY when a confirmed critical/high correctness or security finding exists; "warn" for notable but non-blocking issues; otherwise "pass".
|
||||||
@@ -36,7 +38,7 @@ Rules:
|
|||||||
if (conventions) {
|
if (conventions) {
|
||||||
user += `\nRepo conventions (excerpt from CLAUDE.md / CONTRIBUTING.md — weigh style/design against these):\n---\n${conventions}\n---\n`;
|
user += `\nRepo conventions (excerpt from CLAUDE.md / CONTRIBUTING.md — weigh style/design against these):\n---\n${conventions}\n---\n`;
|
||||||
}
|
}
|
||||||
if (truncated) user += `\nNOTE: the diff below was TRUNCATED at the size cap; judge only what you see and do not penalize for the missing tail.\n`;
|
if (truncated) user += `\nNOTE: this is a partial review (diff truncated) — the diff below was cut at the size cap. Judge only what you see and do not penalize for the missing tail; unseen code is unreviewed, not approved.\n`;
|
||||||
user += `\nUnified diff to review:\n\`\`\`diff\n${diff}\n\`\`\`\n\nReturn the JSON object now.`;
|
user += `\nUnified diff to review:\n\`\`\`diff\n${diff}\n\`\`\`\n\nReturn the JSON object now.`;
|
||||||
return [
|
return [
|
||||||
{ role: 'system', content: system },
|
{ role: 'system', content: system },
|
||||||
|
|||||||
+26
-2
@@ -5,20 +5,44 @@ export class ReviewQueue {
|
|||||||
constructor() {
|
constructor() {
|
||||||
this.repoChains = new Map();
|
this.repoChains = new Map();
|
||||||
this.globalChain = Promise.resolve();
|
this.globalChain = Promise.resolve();
|
||||||
|
this.dedupeGenerations = new Map();
|
||||||
this.pending = 0;
|
this.pending = 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Serialize fn per repo key. fn receives a `withGlobal` helper that
|
// Serialize fn per repo key. fn receives a `withGlobal` helper that
|
||||||
// serializes the wrapped section across ALL repos (use around LLM calls).
|
// serializes the wrapped section across ALL repos (use around LLM calls).
|
||||||
enqueue(repoKey, fn) {
|
enqueue(repoKey, fn, { dedupeKey = null, dedupeKeys = [], onSuperseded = null } = {}) {
|
||||||
const prev = this.repoChains.get(repoKey) || Promise.resolve();
|
const prev = this.repoChains.get(repoKey) || Promise.resolve();
|
||||||
|
const generationKeys = [...new Set([dedupeKey, ...dedupeKeys].filter(Boolean))]
|
||||||
|
.map((key) => `${repoKey}:${key}`);
|
||||||
|
const generations = new Map(generationKeys.map((key) => [
|
||||||
|
key,
|
||||||
|
(this.dedupeGenerations.get(key) || 0) + 1
|
||||||
|
]));
|
||||||
|
for (const [key, generation] of generations) {
|
||||||
|
this.dedupeGenerations.set(key, generation);
|
||||||
|
}
|
||||||
this.pending++;
|
this.pending++;
|
||||||
const next = prev
|
const next = prev
|
||||||
.catch(() => {})
|
.catch(() => {})
|
||||||
.then(() => fn(this.withGlobal.bind(this)))
|
.then(() => {
|
||||||
|
const superseded = [...generations].some(
|
||||||
|
([key, generation]) => this.dedupeGenerations.get(key) !== generation
|
||||||
|
);
|
||||||
|
if (superseded) {
|
||||||
|
onSuperseded?.();
|
||||||
|
return { superseded: true };
|
||||||
|
}
|
||||||
|
return fn(this.withGlobal.bind(this));
|
||||||
|
})
|
||||||
.finally(() => {
|
.finally(() => {
|
||||||
this.pending--;
|
this.pending--;
|
||||||
if (this.repoChains.get(repoKey) === next) this.repoChains.delete(repoKey);
|
if (this.repoChains.get(repoKey) === next) this.repoChains.delete(repoKey);
|
||||||
|
for (const [key, generation] of generations) {
|
||||||
|
if (this.dedupeGenerations.get(key) === generation) {
|
||||||
|
this.dedupeGenerations.delete(key);
|
||||||
|
}
|
||||||
|
}
|
||||||
});
|
});
|
||||||
this.repoChains.set(repoKey, next);
|
this.repoChains.set(repoKey, next);
|
||||||
return next;
|
return next;
|
||||||
|
|||||||
+240
-33
@@ -1,23 +1,51 @@
|
|||||||
// The review pipeline: diff fetch -> conventions -> LLM -> parse -> status +
|
// The review pipeline: diff fetch -> conventions -> LLM -> parse -> status +
|
||||||
// comment -> ledger. Fail-open with visibility: if the LLM/router path dies,
|
// comment -> ledger. Fail-open with visibility: if the LLM/router path dies,
|
||||||
// we post a 'warning' commit status saying review unavailable — never a silent
|
// we post a commit status saying review unavailable — never a silent block,
|
||||||
// block, never a silent pass marked success.
|
// and never a silent pass: the reason always rides in the description and the
|
||||||
|
// error is always recorded in the reviews table.
|
||||||
|
//
|
||||||
|
// That status is 'success', and this is deliberate — it used to be 'warning'.
|
||||||
|
// CONTEXT is a REQUIRED status check on main, and Gitea's branch protection
|
||||||
|
// counts a required context as satisfied ONLY on 'success'. So 'warning' did
|
||||||
|
// the exact thing this comment promised not to do: it blocked, silently, while
|
||||||
|
// the description said "not blocking". Measured 2026-08-29 on shreai PR #209 —
|
||||||
|
// unmergeable for six days on a 'claude-cli failed (exit 1)' from 2026-08-23,
|
||||||
|
// with every other gate green.
|
||||||
|
//
|
||||||
|
// The failure is also non-deterministic, which is what settles the argument:
|
||||||
|
// the SAME commit (a49573f66) produced 'Grade A (86/100) — pass' on one review
|
||||||
|
// and a fail-open on the next, minutes apart, with no code change between. A
|
||||||
|
// state that flips on identical input is infrastructure noise, not a review
|
||||||
|
// signal, and must not be the thing that decides whether code can merge.
|
||||||
|
//
|
||||||
|
// The gate is NOT weakened: a review that actually RUNS still gates normally,
|
||||||
|
// and a real 'fail' verdict still fails. Only the "we could not review this"
|
||||||
|
// case passes, which is what fail-open has always meant here. Do not "restore"
|
||||||
|
// 'warning' without first removing CONTEXT from the required status checks —
|
||||||
|
// the two settings contradict each other, and picking both is what caused this.
|
||||||
|
|
||||||
import { chat } from './router.js';
|
import { chat } from './router.js';
|
||||||
import { extractJson } from './jsonExtract.js';
|
import { extractJson } from './jsonExtract.js';
|
||||||
import { normalizeReview } from './verdict.js';
|
import { normalizeReview, applyTruncationCap } from './verdict.js';
|
||||||
import { parseTrailers } from './trailers.js';
|
import { parseTrailers } from './trailers.js';
|
||||||
import { buildReviewPrompt } from './prompt.js';
|
import { buildReviewPrompt } from './prompt.js';
|
||||||
import { renderScorecardMarkdown } from './render.js';
|
import { renderScorecardMarkdown } from './render.js';
|
||||||
import { insertReview } from './db.js';
|
import { insertReview } from './db.js';
|
||||||
|
import { withCollabRetry } from './access.js';
|
||||||
|
|
||||||
const CONTEXT = 'granthi-review';
|
const CONTEXT = 'granthi-review';
|
||||||
|
const ZERO_SHA = /^0+$/;
|
||||||
|
|
||||||
export async function runReview({ cfg, gitea, db, job, withGlobal, log }) {
|
export async function runReview({ cfg, gitea, adminGitea, db, job, withGlobal, log }) {
|
||||||
const { owner, repo, sha } = job;
|
const { owner, repo, sha } = job;
|
||||||
const fullRepo = `${owner}/${repo}`;
|
const fullRepo = `${owner}/${repo}`;
|
||||||
const dashboardUrl = `${cfg.publicBaseUrl}/review/${owner}/${repo}`;
|
const dashboardUrl = `${cfg.publicBaseUrl}/review/${owner}/${repo}`;
|
||||||
const t0 = Date.now();
|
const t0 = Date.now();
|
||||||
|
// status/comment posters that self-heal a 403 (bot not yet collaborator on
|
||||||
|
// repos auto-wired by the admin default hook) via the admin token, once.
|
||||||
|
const retryCtx = { adminGitea, owner, repo, botUsername: cfg.botUsername, log };
|
||||||
|
const postStatus = (body) => withCollabRetry(() => gitea.postStatus(owner, repo, sha, body), retryCtx);
|
||||||
|
const postComment = (index, md) => withCollabRetry(() => gitea.postIssueComment(owner, repo, index, md), retryCtx);
|
||||||
|
|
||||||
// For PR events the webhook only carries title/body; pull the PR's commits
|
// For PR events the webhook only carries title/body; pull the PR's commits
|
||||||
// so Co-Authored-By trailer attribution sees real commit messages.
|
// so Co-Authored-By trailer attribution sees real commit messages.
|
||||||
@@ -33,26 +61,42 @@ export async function runReview({ cfg, gitea, db, job, withGlobal, log }) {
|
|||||||
|
|
||||||
// 0. pending status (best-effort)
|
// 0. pending status (best-effort)
|
||||||
await safe(log, 'pending-status', () =>
|
await safe(log, 'pending-status', () =>
|
||||||
gitea.postStatus(owner, repo, sha, {
|
postStatus({
|
||||||
state: 'pending', context: CONTEXT,
|
state: 'pending', context: CONTEXT,
|
||||||
description: 'AI review in progress…', target_url: dashboardUrl
|
description: 'AI review in progress…', target_url: dashboardUrl
|
||||||
}));
|
}));
|
||||||
|
|
||||||
// 1. diff
|
// 1. diff — any fetch failure (throw, non-200, or empty body for a job that
|
||||||
let diff = '', truncated = false, diffBytes = 0;
|
// has commits) short-circuits to a non-blocking 'success' status. A diffless
|
||||||
|
// prompt would make the model "review" nothing and pass; never send one.
|
||||||
|
let diff = '', truncated = false, diffBytes = 0, partialDiff = false, partialNote = null;
|
||||||
try {
|
try {
|
||||||
diff = await fetchDiff({ gitea, job });
|
const fetched = await fetchDiff({
|
||||||
|
gitea, job, maxBytes: cfg.maxDiffBytes,
|
||||||
|
log: (m) => log(`[${fullRepo}@${sha.slice(0, 8)}] ${m}`)
|
||||||
|
});
|
||||||
|
diff = fetched.diff;
|
||||||
|
partialDiff = fetched.partial;
|
||||||
|
partialNote = fetched.note;
|
||||||
|
} catch (e) {
|
||||||
|
return await failOpen({
|
||||||
|
cfg, db, job, log, postStatus, dashboardUrl, t0,
|
||||||
|
error: `diff fetch failed: ${e.message}`,
|
||||||
|
description: 'review unavailable — diff fetch failed (not blocking)'
|
||||||
|
});
|
||||||
|
}
|
||||||
|
if (!diff || !diff.trim()) {
|
||||||
|
return await failOpen({
|
||||||
|
cfg, db, job, log, postStatus, dashboardUrl, t0,
|
||||||
|
error: 'diff fetch failed: empty diff body for a non-empty push/PR',
|
||||||
|
description: 'review unavailable — diff fetch failed (not blocking)'
|
||||||
|
});
|
||||||
|
}
|
||||||
diffBytes = Buffer.byteLength(diff);
|
diffBytes = Buffer.byteLength(diff);
|
||||||
if (diffBytes > cfg.maxDiffBytes) {
|
if (diffBytes > cfg.maxDiffBytes) {
|
||||||
diff = Buffer.from(diff).subarray(0, cfg.maxDiffBytes).toString('utf8');
|
diff = Buffer.from(diff).subarray(0, cfg.maxDiffBytes).toString('utf8');
|
||||||
truncated = true;
|
truncated = true;
|
||||||
log(`[${fullRepo}@${sha.slice(0, 8)}] diff truncated ${diffBytes} -> ${cfg.maxDiffBytes} bytes`);
|
log(`[${fullRepo}@${sha.slice(0, 8)}] diff truncated ${diffBytes} -> ${cfg.maxDiffBytes} bytes (partial review)`);
|
||||||
}
|
|
||||||
} catch (e) {
|
|
||||||
return await failOpen({ cfg, gitea, db, job, log, dashboardUrl, t0, error: `diff fetch failed: ${e.message}` });
|
|
||||||
}
|
|
||||||
if (!diff.trim()) {
|
|
||||||
return await failOpen({ cfg, gitea, db, job, log, dashboardUrl, t0, error: 'empty diff' , state: 'success', description: 'empty diff — nothing to review'});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// 2. conventions
|
// 2. conventions
|
||||||
@@ -82,6 +126,7 @@ export async function runReview({ cfg, gitea, db, job, withGlobal, log }) {
|
|||||||
tenantId: cfg.tenantId, timeoutMs: cfg.routerTimeoutMs
|
tenantId: cfg.tenantId, timeoutMs: cfg.routerTimeoutMs
|
||||||
});
|
});
|
||||||
servedModel = out.servedModel;
|
servedModel = out.servedModel;
|
||||||
|
assertServedModelAllowed(cfg.model, servedModel);
|
||||||
const raw = extractJson(out.content);
|
const raw = extractJson(out.content);
|
||||||
if (!raw) {
|
if (!raw) {
|
||||||
log(`[${fullRepo}@${sha.slice(0, 8)}] unparseable model output (served=${servedModel}): ${JSON.stringify(out.content.slice(0, 400))}`);
|
log(`[${fullRepo}@${sha.slice(0, 8)}] unparseable model output (served=${servedModel}): ${JSON.stringify(out.content.slice(0, 400))}`);
|
||||||
@@ -98,9 +143,15 @@ export async function runReview({ cfg, gitea, db, job, withGlobal, log }) {
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
if (!review) {
|
if (!review) {
|
||||||
return await failOpen({ cfg, gitea, db, job, log, dashboardUrl, t0, error: `review unavailable: ${llmError}` });
|
return await failOpen({ cfg, db, job, log, postStatus, dashboardUrl, t0, error: `review unavailable: ${llmError}` });
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A truncated OR partial diff can never yield a clean 'pass' — part of the
|
||||||
|
// change was not reviewed. Cap at 'warn' (a confirmed-critical 'fail' still
|
||||||
|
// fails). Partial = a fallback source covered less than the full range
|
||||||
|
// (e.g. head commit only); reviewing a subset must not read as a full pass.
|
||||||
|
applyTruncationCap(review, truncated || partialDiff);
|
||||||
|
|
||||||
// 4. status + comment
|
// 4. status + comment
|
||||||
const stateByVerdict = { pass: 'success', warn: 'success', fail: 'failure' };
|
const stateByVerdict = { pass: 'success', warn: 'success', fail: 'failure' };
|
||||||
const state = stateByVerdict[review.verdict];
|
const state = stateByVerdict[review.verdict];
|
||||||
@@ -109,14 +160,19 @@ export async function runReview({ cfg, gitea, db, job, withGlobal, log }) {
|
|||||||
warn: `Grade ${review.grade} (${review.overall}/100) — pass with warnings (${review.findings.length} finding(s))`,
|
warn: `Grade ${review.grade} (${review.overall}/100) — pass with warnings (${review.findings.length} finding(s))`,
|
||||||
fail: `Grade ${review.grade} (${review.overall}/100) — BLOCKED: confirmed critical finding (admin merge = override)`
|
fail: `Grade ${review.grade} (${review.overall}/100) — BLOCKED: confirmed critical finding (admin merge = override)`
|
||||||
};
|
};
|
||||||
|
const partialReasons = [
|
||||||
|
...(truncated ? ['diff truncated'] : []),
|
||||||
|
...(partialDiff ? [partialNote || 'incomplete diff coverage'] : [])
|
||||||
|
];
|
||||||
|
const description = partialReasons.length
|
||||||
|
? `partial review (${partialReasons.join('; ')}) — ${descByVerdict[review.verdict]}`
|
||||||
|
: descByVerdict[review.verdict];
|
||||||
await safe(log, 'final-status', () =>
|
await safe(log, 'final-status', () =>
|
||||||
gitea.postStatus(owner, repo, sha, {
|
postStatus({ state, context: CONTEXT, description, target_url: dashboardUrl }));
|
||||||
state, context: CONTEXT, description: descByVerdict[review.verdict], target_url: dashboardUrl
|
|
||||||
}));
|
|
||||||
|
|
||||||
const md = renderScorecardMarkdown({ repo: fullRepo, sha, review, dashboardUrl, truncated });
|
const md = renderScorecardMarkdown({ repo: fullRepo, sha, review, dashboardUrl, truncated: truncated || partialDiff });
|
||||||
if (job.prIndex) {
|
if (job.prIndex) {
|
||||||
await safe(log, 'pr-comment', () => gitea.postIssueComment(owner, repo, job.prIndex, md));
|
await safe(log, 'pr-comment', () => postComment(job.prIndex, md));
|
||||||
} else {
|
} else {
|
||||||
// Gitea has no commit-comment API endpoint; bare pushes get status +
|
// Gitea has no commit-comment API endpoint; bare pushes get status +
|
||||||
// ledger + dashboard only (README documents this).
|
// ledger + dashboard only (README documents this).
|
||||||
@@ -130,31 +186,182 @@ export async function runReview({ cfg, gitea, db, job, withGlobal, log }) {
|
|||||||
pusher: job.pusher, authors: job.authors, trailers,
|
pusher: job.pusher, authors: job.authors, trailers,
|
||||||
axes: review.axes, overall: review.overall, grade: review.grade,
|
axes: review.axes, overall: review.overall, grade: review.grade,
|
||||||
verdict: review.verdict, findings: review.findings,
|
verdict: review.verdict, findings: review.findings,
|
||||||
statusState: state, error: null, diffBytes, truncated,
|
statusState: state, error: null, diffBytes, truncated: truncated || partialDiff,
|
||||||
model: servedModel || cfg.model, durationMs: Date.now() - t0
|
model: servedModel || cfg.model, durationMs: Date.now() - t0
|
||||||
});
|
});
|
||||||
log(`[${fullRepo}@${sha.slice(0, 8)}] reviewed: ${review.verdict} grade=${review.grade} overall=${review.overall} findings=${review.findings.length} in ${Date.now() - t0}ms`);
|
log(`[${fullRepo}@${sha.slice(0, 8)}] reviewed: ${review.verdict} grade=${review.grade} overall=${review.overall} findings=${review.findings.length} in ${Date.now() - t0}ms`);
|
||||||
return { ok: true, verdict: review.verdict };
|
return { ok: true, verdict: review.verdict };
|
||||||
}
|
}
|
||||||
|
|
||||||
async function fetchDiff({ gitea, job }) {
|
// Push reviews cover the FULL push range — never a silent per-commit subset,
|
||||||
const { owner, repo } = job;
|
// so the head status can't claim success for commits that were dropped.
|
||||||
if (job.prIndex) return gitea.getPrDiff(owner, repo, job.prIndex);
|
//
|
||||||
// push: per-commit diffs for the pushed shas (payload-provided), capped later
|
// Primary sources: PR jobs use the API PR diff; range pushes use ONE web
|
||||||
const parts = [];
|
// compare diff (base=before, head=after); new-branch pushes (before = 0000…)
|
||||||
for (const sha of job.commitShas.slice(0, 20)) {
|
// use the head commit's diff.
|
||||||
try { parts.push(await gitea.getCommitDiff(owner, repo, sha)); }
|
//
|
||||||
catch (e) { parts.push(`# diff for ${sha} unavailable: ${e.message}\n`); }
|
// Fallback chain (a 404 or empty body moves to the next source; any other
|
||||||
|
// error — 5xx, network, timeout — propagates immediately, those are forge
|
||||||
|
// failures and trying other refs would only hide them):
|
||||||
|
// PR job: pulls/{n}.diff → git/commits/{head}.diff
|
||||||
|
// range push:
|
||||||
|
// 1. web compare {before}...{after}.diff (404s when `before` is
|
||||||
|
// unknown — force-push/rebase — AND, probed live on Gitea 1.27.1, for
|
||||||
|
// ALL refs on private repos: the web route ignores token auth)
|
||||||
|
// 2. web compare {defaultBranch}...{after}.diff (covers unknown-`before`
|
||||||
|
// where the web route itself works, e.g. public repos)
|
||||||
|
// 3. API compare {before}...{after} (JSON commit list; this endpoint DOES
|
||||||
|
// honor token auth) → concatenated per-commit git/commits/{sha}.diff,
|
||||||
|
// falling back to the webhook's commitShas for the list. Stops once
|
||||||
|
// past maxBytes — review.js then truncates and caps the verdict.
|
||||||
|
// 4. git/commits/{after}.diff (head only, unless 3 already tried it)
|
||||||
|
// Only when every source is exhausted does the error propagate to fail-open,
|
||||||
|
// listing every URL tried.
|
||||||
|
// Returns { diff, partial, note }. `partial: true` means the source covered
|
||||||
|
// LESS than the job's full range (head-only fallback, capped/incomplete
|
||||||
|
// per-commit reconstruction) — review.js caps the verdict at 'warn' so a
|
||||||
|
// subset review can never read as a full pass.
|
||||||
|
const SHA_RE = /^[0-9a-f]{7,40}$/i;
|
||||||
|
const MAX_COMMIT_DIFFS = 20;
|
||||||
|
const full = (diff) => ({ diff, partial: false, note: null });
|
||||||
|
const part = (diff, note) => ({ diff, partial: true, note });
|
||||||
|
|
||||||
|
export async function fetchDiff({ gitea, job, log = () => {}, maxBytes = Infinity }) {
|
||||||
|
const { owner, repo, sha } = job;
|
||||||
|
const tried = [];
|
||||||
|
|
||||||
|
// Try one diff source: diff text on success, null on 404/empty (recorded
|
||||||
|
// in `tried`), throw on anything else.
|
||||||
|
const attempt = async (label, fn) => {
|
||||||
|
try {
|
||||||
|
const out = await fn();
|
||||||
|
if (out && out.trim()) return out;
|
||||||
|
tried.push(`${label}: empty diff body`);
|
||||||
|
return null;
|
||||||
|
} catch (e) {
|
||||||
|
if (e.status !== 404) throw e;
|
||||||
|
tried.push(e.message);
|
||||||
|
return null;
|
||||||
}
|
}
|
||||||
return parts.join('\n');
|
};
|
||||||
|
|
||||||
|
let diff;
|
||||||
|
if (job.prIndex) {
|
||||||
|
diff = await attempt(`pr #${job.prIndex} diff`, () => gitea.getPrDiff(owner, repo, job.prIndex));
|
||||||
|
if (diff) return full(diff);
|
||||||
|
diff = await attempt(`commit ${sha.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, sha));
|
||||||
|
if (diff) {
|
||||||
|
log(`diff via fallback: head commit ${sha.slice(0, 8)} (PR #${job.prIndex} diff unavailable) — PARTIAL`);
|
||||||
|
return part(diff, `head commit only, PR #${job.prIndex} diff unavailable`);
|
||||||
|
}
|
||||||
|
throw allDiffSourcesFailed(tried);
|
||||||
}
|
}
|
||||||
|
|
||||||
async function failOpen({ cfg, gitea, db, job, log, dashboardUrl, t0, error, state = 'warning', description }) {
|
if (job.before && !ZERO_SHA.test(job.before)) {
|
||||||
|
// 1. full-range web compare (the normal path)
|
||||||
|
diff = await attempt(`compare ${job.before.slice(0, 8)}...${sha.slice(0, 8)} diff`, () => gitea.getCompareDiff(owner, repo, job.before, sha));
|
||||||
|
if (diff) return full(diff);
|
||||||
|
|
||||||
|
// 2. range against the repo default branch (base the server surely has).
|
||||||
|
// Covers a SUPERSET of the push range (from the merge-base), so complete.
|
||||||
|
let defaultBranch = job.defaultBranch;
|
||||||
|
if (!defaultBranch) {
|
||||||
|
// Optional lookup: only a 404 (repo gone) is a recordable miss; auth
|
||||||
|
// and server failures must propagate, not silently skip a source.
|
||||||
|
try { defaultBranch = (await gitea.getRepo(owner, repo))?.default_branch; }
|
||||||
|
catch (e) {
|
||||||
|
if (e.status !== 404) throw e;
|
||||||
|
tried.push(e.message);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if (defaultBranch) {
|
||||||
|
diff = await attempt(`compare ${defaultBranch}...${sha.slice(0, 8)} diff`, () => gitea.getCompareDiff(owner, repo, defaultBranch, sha));
|
||||||
|
if (diff) {
|
||||||
|
log(`diff via fallback: compare ${defaultBranch}...${sha.slice(0, 8)}`);
|
||||||
|
return full(diff);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// 3. reconstruct the range from the API: commit list, then per-commit diffs
|
||||||
|
let shas = null;
|
||||||
|
try {
|
||||||
|
const cmp = await gitea.compare(owner, repo, job.before, sha);
|
||||||
|
const list = (cmp?.commits || []).map(c => c?.sha).filter(Boolean);
|
||||||
|
if (list.length) shas = list;
|
||||||
|
else tried.push(`api compare ${job.before.slice(0, 8)}...${sha.slice(0, 8)}: no commits`);
|
||||||
|
} catch (e) {
|
||||||
|
if (e.status !== 404) throw e;
|
||||||
|
tried.push(e.message);
|
||||||
|
}
|
||||||
|
if (!shas && Array.isArray(job.commitShas) && job.commitShas.length) shas = job.commitShas;
|
||||||
|
if (shas) {
|
||||||
|
// Webhook/API-supplied list: dedupe, drop anything that isn't a sha,
|
||||||
|
// and hard-cap the request chain so a hostile or huge payload cannot
|
||||||
|
// fan out into hundreds of sequential fetches.
|
||||||
|
const cleaned = [...new Set(shas)].filter((s) => typeof s === 'string' && SHA_RE.test(s));
|
||||||
|
const capped = cleaned.slice(0, MAX_COMMIT_DIFFS);
|
||||||
|
const parts = [];
|
||||||
|
let total = 0, hitByteCap = false;
|
||||||
|
for (const s of capped) {
|
||||||
|
const d = await attempt(`commit ${s.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, s));
|
||||||
|
if (d) { parts.push(d); total += Buffer.byteLength(d); }
|
||||||
|
if (total > maxBytes) { hitByteCap = true; break; } // review.js truncates + caps the verdict
|
||||||
|
}
|
||||||
|
if (total > 0) {
|
||||||
|
const incomplete = hitByteCap || parts.length < capped.length || cleaned.length > capped.length;
|
||||||
|
log(`diff via fallback: ${parts.length}/${cleaned.length} per-commit diff(s) over ${job.before.slice(0, 8)}...${sha.slice(0, 8)}${incomplete ? ' — PARTIAL' : ''}`);
|
||||||
|
const joined = parts.join('\n');
|
||||||
|
return incomplete
|
||||||
|
? part(joined, `per-commit reconstruction covered ${parts.length}/${cleaned.length} commit(s)`)
|
||||||
|
: full(joined);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// 4. last resort: head commit alone (skip if step 3 already tried it)
|
||||||
|
if (!shas || !shas.includes(sha)) {
|
||||||
|
diff = await attempt(`commit ${sha.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, sha));
|
||||||
|
if (diff) {
|
||||||
|
log(`diff via fallback: head commit ${sha.slice(0, 8)} only — PARTIAL`);
|
||||||
|
return part(diff, `head commit only, range ${job.before.slice(0, 8)}...${sha.slice(0, 8)} unrecoverable`);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
throw allDiffSourcesFailed(tried);
|
||||||
|
}
|
||||||
|
|
||||||
|
// new-branch push (before = 0000… or missing): head commit diff is the
|
||||||
|
// long-standing contract for this event shape — complete by definition.
|
||||||
|
diff = await attempt(`commit ${sha.slice(0, 8)} diff`, () => gitea.getCommitDiff(owner, repo, sha));
|
||||||
|
if (diff) return full(diff);
|
||||||
|
throw allDiffSourcesFailed(tried);
|
||||||
|
}
|
||||||
|
|
||||||
|
export function assertServedModelAllowed(requestedModel, servedModel) {
|
||||||
|
if (!isLocalModel(requestedModel)) return;
|
||||||
|
if (!servedModel) return;
|
||||||
|
if (servedModel === requestedModel) return;
|
||||||
|
throw new Error(`local model contract violated: requested ${requestedModel}, served ${servedModel}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
function isLocalModel(model) {
|
||||||
|
return /^ollama(?:-remote)?\//.test(String(model || ''));
|
||||||
|
}
|
||||||
|
|
||||||
|
function allDiffSourcesFailed(tried) {
|
||||||
|
let msg = tried.join(' | ');
|
||||||
|
if (msg.length > 1500) msg = msg.slice(0, 1500) + ` …(+${tried.length} sources)`;
|
||||||
|
return new Error(`all diff sources failed: ${msg}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
// state defaults to 'success' because CONTEXT is a required check — see the
|
||||||
|
// header. 'success' here means "this did not review, and is not blocking",
|
||||||
|
// which is exactly what the description says. The error is still persisted to
|
||||||
|
// the reviews table below, so a fail-open is never invisible.
|
||||||
|
async function failOpen({ cfg, db, job, log, postStatus, dashboardUrl, t0, error, state = 'success', description }) {
|
||||||
const { owner, repo, sha } = job;
|
const { owner, repo, sha } = job;
|
||||||
const fullRepo = `${owner}/${repo}`;
|
const fullRepo = `${owner}/${repo}`;
|
||||||
log(`[${fullRepo}@${sha.slice(0, 8)}] fail-open: ${error}`);
|
log(`[${fullRepo}@${sha.slice(0, 8)}] fail-open: ${error}`);
|
||||||
await safe(log, 'fail-open-status', () =>
|
await safe(log, 'fail-open-status', () =>
|
||||||
gitea.postStatus(owner, repo, sha, {
|
postStatus({
|
||||||
state, context: CONTEXT,
|
state, context: CONTEXT,
|
||||||
description: description || `review unavailable — not blocking (${error.slice(0, 180)})`,
|
description: description || `review unavailable — not blocking (${error.slice(0, 180)})`,
|
||||||
target_url: dashboardUrl
|
target_url: dashboardUrl
|
||||||
|
|||||||
+54
-2
@@ -2,7 +2,56 @@
|
|||||||
// Real model answer lives in .message.content; top-level .content can carry an
|
// Real model answer lives in .message.content; top-level .content can carry an
|
||||||
// upstream-down apology — always prefer .message.content.
|
// upstream-down apology — always prefer .message.content.
|
||||||
|
|
||||||
export async function chat({ routerUrl, model, messages, agentId, sessionId, tenantId, timeoutMs }) {
|
import { execFile } from 'node:child_process';
|
||||||
|
import { promisify } from 'node:util';
|
||||||
|
|
||||||
|
const execFileAsync = promisify(execFile);
|
||||||
|
|
||||||
|
async function chatWithClaudeCli({ model, messages, timeoutMs, claudeBin = 'claude' }) {
|
||||||
|
const modelId = model.slice('claude-cli/'.length);
|
||||||
|
if (!modelId) throw new Error('claude-cli model id is required');
|
||||||
|
|
||||||
|
const prompt = messages
|
||||||
|
.map(({ role, content }) => `${String(role || 'user').toUpperCase()}:\n${String(content || '')}`)
|
||||||
|
.join('\n\n');
|
||||||
|
const env = { ...process.env };
|
||||||
|
delete env.ANTHROPIC_API_KEY;
|
||||||
|
delete env.ANTHROPIC_TOKEN;
|
||||||
|
delete env.ANTHROPIC_AUTH_TOKEN;
|
||||||
|
|
||||||
|
let stdout;
|
||||||
|
try {
|
||||||
|
({ stdout } = await execFileAsync(claudeBin, [
|
||||||
|
'-p', prompt,
|
||||||
|
'--model', modelId,
|
||||||
|
'--output-format', 'json',
|
||||||
|
'--tools', '',
|
||||||
|
'--permission-mode', 'dontAsk',
|
||||||
|
'--no-session-persistence'
|
||||||
|
], {
|
||||||
|
env,
|
||||||
|
timeout: timeoutMs,
|
||||||
|
killSignal: 'SIGTERM',
|
||||||
|
maxBuffer: 4 * 1024 * 1024
|
||||||
|
}));
|
||||||
|
} catch (error) {
|
||||||
|
const timedOut = error?.killed || error?.signal === 'SIGTERM' || error?.code === 'ETIMEDOUT';
|
||||||
|
const exitCode = Number.isInteger(error?.code) ? ` (exit ${error.code})` : '';
|
||||||
|
throw new Error(`claude-cli ${timedOut ? 'timed out' : 'failed'}${exitCode}`);
|
||||||
|
}
|
||||||
|
const data = JSON.parse(stdout);
|
||||||
|
const content = data?.result ?? data?.content;
|
||||||
|
if (typeof content !== 'string' || !content.trim()) {
|
||||||
|
throw new Error('claude-cli returned empty content');
|
||||||
|
}
|
||||||
|
return { content, servedModel: model };
|
||||||
|
}
|
||||||
|
|
||||||
|
export async function chat(options) {
|
||||||
|
const { routerUrl, model, messages, agentId, sessionId, tenantId, timeoutMs } = options;
|
||||||
|
if (model.startsWith('claude-cli/')) {
|
||||||
|
return chatWithClaudeCli(options);
|
||||||
|
}
|
||||||
const res = await fetch(`${routerUrl.replace(/\/$/, '')}/v1/chat`, {
|
const res = await fetch(`${routerUrl.replace(/\/$/, '')}/v1/chat`, {
|
||||||
method: 'POST',
|
method: 'POST',
|
||||||
headers: { 'Content-Type': 'application/json' },
|
headers: { 'Content-Type': 'application/json' },
|
||||||
@@ -13,7 +62,10 @@ export async function chat({ routerUrl, model, messages, agentId, sessionId, ten
|
|||||||
// and return non-review text. The domain preflight (agent_mismatch 400)
|
// and return non-review text. The domain preflight (agent_mismatch 400)
|
||||||
// still applies per agent tier; the pipeline's attempt-2 fallback agent
|
// still applies per agent tier; the pipeline's attempt-2 fallback agent
|
||||||
// covers that.
|
// covers that.
|
||||||
body: JSON.stringify({ model, messages, agentId, sessionId, tenantId, stream: false, tools: false, raw: true }),
|
body: JSON.stringify({
|
||||||
|
model, messages, agentId, sessionId, tenantId,
|
||||||
|
stream: false, tools: false, raw: true, fallbackModels: []
|
||||||
|
}),
|
||||||
signal: AbortSignal.timeout(timeoutMs)
|
signal: AbortSignal.timeout(timeoutMs)
|
||||||
});
|
});
|
||||||
if (!res.ok) {
|
if (!res.ok) {
|
||||||
|
|||||||
+18
-2
@@ -7,6 +7,10 @@ const BLOCKING_AXES = new Set(['correctness', 'security']);
|
|||||||
|
|
||||||
export function isBlockingFinding(f) {
|
export function isBlockingFinding(f) {
|
||||||
if (!f || typeof f !== 'object') return false;
|
if (!f || typeof f !== 'object') return false;
|
||||||
|
// A finding the diff itself resolves describes the PRE-change state; it
|
||||||
|
// cannot block the change that fixes it (models routinely narrate the fixed
|
||||||
|
// bug as a critical/confirmed finding, which failed correct fixes).
|
||||||
|
if (f.resolved_by_this_change === true) return false;
|
||||||
const sev = String(f.severity || '').toLowerCase();
|
const sev = String(f.severity || '').toLowerCase();
|
||||||
const conf = String(f.confidence || '').toLowerCase();
|
const conf = String(f.confidence || '').toLowerCase();
|
||||||
if (!BLOCKING_SEVERITIES.has(sev)) return false;
|
if (!BLOCKING_SEVERITIES.has(sev)) return false;
|
||||||
@@ -21,12 +25,16 @@ export function deriveVerdict(review) {
|
|||||||
const findings = Array.isArray(review?.findings) ? review.findings : [];
|
const findings = Array.isArray(review?.findings) ? review.findings : [];
|
||||||
if (findings.some(isBlockingFinding)) return 'fail';
|
if (findings.some(isBlockingFinding)) return 'fail';
|
||||||
const hasWarn = findings.some(f => {
|
const hasWarn = findings.some(f => {
|
||||||
|
if (f?.resolved_by_this_change === true) return false;
|
||||||
const sev = String(f?.severity || '').toLowerCase();
|
const sev = String(f?.severity || '').toLowerCase();
|
||||||
return sev === 'critical' || sev === 'high' || sev === 'medium';
|
return sev === 'critical' || sev === 'high' || sev === 'medium';
|
||||||
});
|
});
|
||||||
if (hasWarn) return 'warn';
|
if (hasWarn) return 'warn';
|
||||||
// trust the model's verdict if it said warn without matching findings
|
// Trust the model's verdict if it said warn without matching findings —
|
||||||
if (review?.verdict === 'warn') return 'warn';
|
// unless every finding it gave is resolved_by_this_change, in which case its
|
||||||
|
// warn is about the pre-change state and the resulting code is clean.
|
||||||
|
const allResolved = findings.length > 0 && findings.every(f => f?.resolved_by_this_change === true);
|
||||||
|
if (review?.verdict === 'warn' && !allResolved) return 'warn';
|
||||||
return 'pass';
|
return 'pass';
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -49,6 +57,7 @@ export function normalizeReview(raw) {
|
|||||||
severity: pick(String(f?.severity || '').toLowerCase(), ['critical', 'high', 'medium', 'low'], 'low'),
|
severity: pick(String(f?.severity || '').toLowerCase(), ['critical', 'high', 'medium', 'low'], 'low'),
|
||||||
confidence: pick(String(f?.confidence || '').toLowerCase(), ['confirmed', 'plausible'], 'plausible'),
|
confidence: pick(String(f?.confidence || '').toLowerCase(), ['confirmed', 'plausible'], 'plausible'),
|
||||||
axis: f?.axis ? String(f.axis).toLowerCase() : null,
|
axis: f?.axis ? String(f.axis).toLowerCase() : null,
|
||||||
|
resolved_by_this_change: f?.resolved_by_this_change === true,
|
||||||
detail: String(f?.detail ?? '').slice(0, 2000)
|
detail: String(f?.detail ?? '').slice(0, 2000)
|
||||||
}));
|
}));
|
||||||
const review = {
|
const review = {
|
||||||
@@ -62,6 +71,13 @@ export function normalizeReview(raw) {
|
|||||||
return review;
|
return review;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A truncated diff means part of the change was never reviewed: a clean
|
||||||
|
// 'pass' cannot be claimed. Cap at 'warn'; a confirmed-critical 'fail' stands.
|
||||||
|
export function applyTruncationCap(review, truncated) {
|
||||||
|
if (truncated && review && review.verdict === 'pass') review.verdict = 'warn';
|
||||||
|
return review;
|
||||||
|
}
|
||||||
|
|
||||||
function clampScore(v) {
|
function clampScore(v) {
|
||||||
const n = Number(v);
|
const n = Number(v);
|
||||||
if (!Number.isFinite(n)) return 0;
|
if (!Number.isFinite(n)) return 0;
|
||||||
|
|||||||
@@ -27,6 +27,7 @@ export function jobFromWebhook(event, payload) {
|
|||||||
sha: after,
|
sha: after,
|
||||||
ref: payload.ref,
|
ref: payload.ref,
|
||||||
before: payload.before,
|
before: payload.before,
|
||||||
|
defaultBranch: payload.repository?.default_branch || null,
|
||||||
prIndex: null,
|
prIndex: null,
|
||||||
prTitle: null,
|
prTitle: null,
|
||||||
pusher: payload.pusher?.username || payload.pusher?.login || null,
|
pusher: payload.pusher?.username || payload.pusher?.login || null,
|
||||||
@@ -48,6 +49,7 @@ export function jobFromWebhook(event, payload) {
|
|||||||
sha: pr.head?.sha,
|
sha: pr.head?.sha,
|
||||||
ref: pr.head?.ref,
|
ref: pr.head?.ref,
|
||||||
before: null,
|
before: null,
|
||||||
|
defaultBranch: payload.repository?.default_branch || null,
|
||||||
prIndex: pr.number,
|
prIndex: pr.number,
|
||||||
prTitle: pr.title || null,
|
prTitle: pr.title || null,
|
||||||
pusher: payload.sender?.username || payload.sender?.login || null,
|
pusher: payload.sender?.username || payload.sender?.login || null,
|
||||||
|
|||||||
+42
-4
@@ -1,6 +1,6 @@
|
|||||||
import { createServer } from 'node:http';
|
import { createServer } from 'node:http';
|
||||||
import { loadConfig } from './config.js';
|
import { loadConfig } from './config.js';
|
||||||
import { openDb, listReviews } from './lib/db.js';
|
import { openDb, listReviews, enqueueJob, startJob, finishJob, recoverPendingJobs } from './lib/db.js';
|
||||||
import { GiteaClient } from './lib/gitea.js';
|
import { GiteaClient } from './lib/gitea.js';
|
||||||
import { ReviewQueue } from './lib/queue.js';
|
import { ReviewQueue } from './lib/queue.js';
|
||||||
import { verifySignature, jobFromWebhook } from './lib/webhook.js';
|
import { verifySignature, jobFromWebhook } from './lib/webhook.js';
|
||||||
@@ -24,8 +24,46 @@ if (!cfg.webhookSecret) { console.error('missing webhookSecret in config'); proc
|
|||||||
|
|
||||||
const db = openDb(cfg.dbPath);
|
const db = openDb(cfg.dbPath);
|
||||||
const gitea = new GiteaClient({ baseUrl: cfg.forgeBaseUrl, token: cfg.botToken });
|
const gitea = new GiteaClient({ baseUrl: cfg.forgeBaseUrl, token: cfg.botToken });
|
||||||
|
// admin-scoped client: only used to self-heal bot collaborator access on 403
|
||||||
|
const adminGitea = cfg.adminToken
|
||||||
|
? new GiteaClient({ baseUrl: cfg.forgeBaseUrl, token: cfg.adminToken })
|
||||||
|
: null;
|
||||||
const queue = new ReviewQueue();
|
const queue = new ReviewQueue();
|
||||||
|
|
||||||
|
// Run one persisted job through the pipeline, marking the queue row done
|
||||||
|
// whatever the outcome (runReview itself posts a final or fail-open status).
|
||||||
|
function dispatchJob(queueRowId, job) {
|
||||||
|
return queue.enqueue(`${job.owner}/${job.repo}`, (withGlobal) =>
|
||||||
|
{
|
||||||
|
startJob(db, queueRowId);
|
||||||
|
return runReview({ cfg, gitea, adminGitea, db, job, withGlobal, log });
|
||||||
|
},
|
||||||
|
{
|
||||||
|
dedupeKeys: [
|
||||||
|
job.prIndex != null ? `pr:${job.prIndex}` : null,
|
||||||
|
`sha:${job.sha}`
|
||||||
|
].filter(Boolean)
|
||||||
|
}
|
||||||
|
).then(
|
||||||
|
(r) => {
|
||||||
|
if (r?.superseded) return finishJob(db, queueRowId, 'superseded');
|
||||||
|
return finishJob(db, queueRowId, r?.ok ? (r.verdict || 'ok') : `error:${String(r?.error || '').slice(0, 200)}`);
|
||||||
|
},
|
||||||
|
(e) => {
|
||||||
|
log(`review pipeline error for ${job.owner}/${job.repo}@${job.sha}: ${e.stack || e.message}`);
|
||||||
|
finishJob(db, queueRowId, `crash:${String(e.message || e).slice(0, 200)}`);
|
||||||
|
}
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Startup reconciliation: re-enqueue persisted jobs a previous process never
|
||||||
|
// finished (their commit status is stuck at 'pending' otherwise).
|
||||||
|
const recovered = recoverPendingJobs(db);
|
||||||
|
if (recovered.length) {
|
||||||
|
log(`recovering ${recovered.length} unfinished review job(s) from previous run`);
|
||||||
|
for (const { id, job } of recovered) dispatchJob(id, job);
|
||||||
|
}
|
||||||
|
|
||||||
const server = createServer((req, res) => {
|
const server = createServer((req, res) => {
|
||||||
const url = new URL(req.url, 'http://x');
|
const url = new URL(req.url, 'http://x');
|
||||||
try {
|
try {
|
||||||
@@ -78,11 +116,11 @@ function handleWebhook(req, res) {
|
|||||||
return json(res, 200, { accepted: false, reason: 'not reviewable' });
|
return json(res, 200, { accepted: false, reason: 'not reviewable' });
|
||||||
}
|
}
|
||||||
log(`webhook: accepted ${job.event} ${job.owner}/${job.repo}@${String(job.sha).slice(0, 10)}${job.prIndex ? ' PR#' + job.prIndex : ''}`);
|
log(`webhook: accepted ${job.event} ${job.owner}/${job.repo}@${String(job.sha).slice(0, 10)}${job.prIndex ? ' PR#' + job.prIndex : ''}`);
|
||||||
|
// persist BEFORE acking/enqueueing so a restart can re-run it, then
|
||||||
// respond immediately; review runs async on the queue
|
// respond immediately; review runs async on the queue
|
||||||
|
const queueRowId = enqueueJob(db, job);
|
||||||
json(res, 202, { accepted: true });
|
json(res, 202, { accepted: true });
|
||||||
queue.enqueue(`${job.owner}/${job.repo}`, (withGlobal) =>
|
dispatchJob(queueRowId, job);
|
||||||
runReview({ cfg, gitea, db, job, withGlobal, log })
|
|
||||||
).catch(e => log(`review pipeline error for ${job.owner}/${job.repo}@${job.sha}: ${e.stack || e.message}`));
|
|
||||||
});
|
});
|
||||||
req.on('error', e => log('webhook req error:', e.message));
|
req.on('error', e => log('webhook req error:', e.message));
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -0,0 +1,84 @@
|
|||||||
|
import { test } from 'node:test';
|
||||||
|
import assert from 'node:assert/strict';
|
||||||
|
import { withCollabRetry } from '../src/lib/access.js';
|
||||||
|
|
||||||
|
const err = (status, msg = `http ${status}`) => Object.assign(new Error(msg), { status });
|
||||||
|
const noLog = () => {};
|
||||||
|
|
||||||
|
function adminStub() {
|
||||||
|
const calls = [];
|
||||||
|
return {
|
||||||
|
calls,
|
||||||
|
addCollaborator(owner, repo, username, permission) {
|
||||||
|
calls.push({ owner, repo, username, permission });
|
||||||
|
return Promise.resolve();
|
||||||
|
}
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
test('403 adds collaborator (write) then retries once and succeeds', async () => {
|
||||||
|
const admin = adminStub();
|
||||||
|
let attempts = 0;
|
||||||
|
const out = await withCollabRetry(async () => {
|
||||||
|
attempts++;
|
||||||
|
if (attempts === 1) throw err(403);
|
||||||
|
return 'ok';
|
||||||
|
}, { adminGitea: admin, owner: 'nirpa', repo: 'newrepo', botUsername: 'shre-reviewer', log: noLog });
|
||||||
|
assert.equal(out, 'ok');
|
||||||
|
assert.equal(attempts, 2);
|
||||||
|
assert.deepEqual(admin.calls, [{ owner: 'nirpa', repo: 'newrepo', username: 'shre-reviewer', permission: 'write' }]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('non-403 errors are rethrown without touching collaborators', async () => {
|
||||||
|
const admin = adminStub();
|
||||||
|
await assert.rejects(
|
||||||
|
() => withCollabRetry(() => Promise.reject(err(500)), {
|
||||||
|
adminGitea: admin, owner: 'o', repo: 'r', botUsername: 'b', log: noLog
|
||||||
|
}),
|
||||||
|
/http 500/
|
||||||
|
);
|
||||||
|
assert.equal(admin.calls.length, 0);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('403 with no admin client rethrows the original 403', async () => {
|
||||||
|
await assert.rejects(
|
||||||
|
() => withCollabRetry(() => Promise.reject(err(403)), {
|
||||||
|
adminGitea: null, owner: 'o', repo: 'r', botUsername: 'b', log: noLog
|
||||||
|
}),
|
||||||
|
(e) => e.status === 403
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('retry happens exactly once: second 403 propagates', async () => {
|
||||||
|
const admin = adminStub();
|
||||||
|
let attempts = 0;
|
||||||
|
await assert.rejects(
|
||||||
|
() => withCollabRetry(() => { attempts++; return Promise.reject(err(403)); }, {
|
||||||
|
adminGitea: admin, owner: 'o', repo: 'r', botUsername: 'b', log: noLog
|
||||||
|
}),
|
||||||
|
(e) => e.status === 403
|
||||||
|
);
|
||||||
|
assert.equal(attempts, 2);
|
||||||
|
assert.equal(admin.calls.length, 1);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('addCollaborator failure propagates (no infinite loop)', async () => {
|
||||||
|
const admin = {
|
||||||
|
addCollaborator: () => Promise.reject(err(422, 'bad permission'))
|
||||||
|
};
|
||||||
|
let attempts = 0;
|
||||||
|
await assert.rejects(
|
||||||
|
() => withCollabRetry(() => { attempts++; return Promise.reject(err(403)); }, {
|
||||||
|
adminGitea: admin, owner: 'o', repo: 'r', botUsername: 'b', log: noLog
|
||||||
|
}),
|
||||||
|
/bad permission/
|
||||||
|
);
|
||||||
|
assert.equal(attempts, 1);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('success path never consults the admin client', async () => {
|
||||||
|
const out = await withCollabRetry(() => Promise.resolve(42), {
|
||||||
|
adminGitea: undefined, owner: 'o', repo: 'r', botUsername: 'b', log: noLog
|
||||||
|
});
|
||||||
|
assert.equal(out, 42);
|
||||||
|
});
|
||||||
@@ -0,0 +1,248 @@
|
|||||||
|
import { test } from 'node:test';
|
||||||
|
import assert from 'node:assert/strict';
|
||||||
|
import { fetchDiff } from '../src/lib/review.js';
|
||||||
|
|
||||||
|
function giteaStub() {
|
||||||
|
const calls = [];
|
||||||
|
return {
|
||||||
|
calls,
|
||||||
|
getPrDiff: (o, r, i) => { calls.push(['pr', o, r, i]); return Promise.resolve('PRDIFF'); },
|
||||||
|
getCommitDiff: (o, r, sha) => { calls.push(['commit', o, r, sha]); return Promise.resolve('HEADDIFF'); },
|
||||||
|
getCompareDiff: (o, r, before, after) => { calls.push(['compare', o, r, before, after]); return Promise.resolve('RANGEDIFF'); }
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
const HEAD = 'a'.repeat(40);
|
||||||
|
const BEFORE = 'b'.repeat(40);
|
||||||
|
|
||||||
|
test('PR jobs use the PR diff', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
const out = await fetchDiff({ gitea: g, job: { owner: 'o', repo: 'r', prIndex: 5, sha: HEAD, before: null } });
|
||||||
|
assert.equal(out.diff, 'PRDIFF');
|
||||||
|
assert.equal(out.partial, false);
|
||||||
|
assert.deepEqual(g.calls, [['pr', 'o', 'r', 5]]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('push uses ONE compare diff over the full before...after range', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
const out = await fetchDiff({
|
||||||
|
gitea: g,
|
||||||
|
job: {
|
||||||
|
owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE,
|
||||||
|
// 25 commits in the payload: must NOT fall back to per-commit slicing
|
||||||
|
commitShas: Array.from({ length: 25 }, (_, i) => String(i).padStart(40, '0'))
|
||||||
|
}
|
||||||
|
});
|
||||||
|
assert.equal(out.diff, 'RANGEDIFF');
|
||||||
|
assert.equal(out.partial, false);
|
||||||
|
assert.deepEqual(g.calls, [['compare', 'o', 'r', BEFORE, HEAD]]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('new-branch push (before = zero sha) falls back to head commit diff', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
const out = await fetchDiff({
|
||||||
|
gitea: g,
|
||||||
|
job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: '0'.repeat(40), commitShas: [HEAD] }
|
||||||
|
});
|
||||||
|
assert.equal(out.diff, 'HEADDIFF');
|
||||||
|
assert.equal(out.partial, false);
|
||||||
|
assert.deepEqual(g.calls, [['commit', 'o', 'r', HEAD]]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('missing before also falls back to head commit diff', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
const out = await fetchDiff({
|
||||||
|
gitea: g,
|
||||||
|
job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: null, commitShas: [HEAD] }
|
||||||
|
});
|
||||||
|
assert.equal(out.diff, 'HEADDIFF');
|
||||||
|
assert.equal(out.partial, false);
|
||||||
|
assert.deepEqual(g.calls, [['commit', 'o', 'r', HEAD]]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('compare failure propagates (no silent placeholder diff)', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
g.getCompareDiff = () => Promise.reject(new Error('boom 500'));
|
||||||
|
await assert.rejects(
|
||||||
|
() => fetchDiff({ gitea: g, job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE } }),
|
||||||
|
/boom 500/
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
// ---- fallback chain (compare 404: force-push, rebase-then-push, or the web
|
||||||
|
// ---- compare route ignoring token auth on private repos) ----
|
||||||
|
|
||||||
|
function nf(path) {
|
||||||
|
const e = new Error(`gitea GET ${path} -> 404: Not found.`);
|
||||||
|
e.status = 404;
|
||||||
|
return e;
|
||||||
|
}
|
||||||
|
|
||||||
|
test('compare 404 falls back to default-branch compare', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
const real = g.getCompareDiff;
|
||||||
|
g.getCompareDiff = (o, r, before, after) => before === BEFORE
|
||||||
|
? Promise.reject(nf(`/o/r/compare/${before}...${after}.diff`))
|
||||||
|
: real(o, r, before, after);
|
||||||
|
const out = await fetchDiff({
|
||||||
|
gitea: g,
|
||||||
|
job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' }
|
||||||
|
});
|
||||||
|
assert.equal(out.diff, 'RANGEDIFF');
|
||||||
|
assert.equal(out.partial, false);
|
||||||
|
assert.deepEqual(g.calls.at(-1), ['compare', 'o', 'r', 'main', HEAD]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('compare 404 without defaultBranch on the job looks it up via getRepo', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
const real = g.getCompareDiff;
|
||||||
|
g.getCompareDiff = (o, r, before, after) => before === BEFORE
|
||||||
|
? Promise.reject(nf(`/o/r/compare/${before}...${after}.diff`))
|
||||||
|
: real(o, r, before, after);
|
||||||
|
g.getRepo = () => Promise.resolve({ default_branch: 'develop' });
|
||||||
|
const out = await fetchDiff({
|
||||||
|
gitea: g,
|
||||||
|
job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE }
|
||||||
|
});
|
||||||
|
assert.equal(out.diff, 'RANGEDIFF');
|
||||||
|
assert.equal(out.partial, false);
|
||||||
|
assert.deepEqual(g.calls.at(-1), ['compare', 'o', 'r', 'develop', HEAD]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('both compares 404 -> API commit list -> concatenated per-commit diffs', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
const C1 = 'c'.repeat(40), C2 = 'd'.repeat(40);
|
||||||
|
g.getCompareDiff = (o, r, before, after) =>
|
||||||
|
Promise.reject(nf(`/o/r/compare/${before}...${after}.diff`));
|
||||||
|
g.compare = (o, r, before, after) => {
|
||||||
|
g.calls.push(['apicompare', o, r, before, after]);
|
||||||
|
return Promise.resolve({ total_commits: 2, commits: [{ sha: C1 }, { sha: C2 }] });
|
||||||
|
};
|
||||||
|
g.getCommitDiff = (o, r, sha) => {
|
||||||
|
g.calls.push(['commit', o, r, sha]);
|
||||||
|
return Promise.resolve(`DIFF(${sha.slice(0, 1)})`);
|
||||||
|
};
|
||||||
|
const out = await fetchDiff({
|
||||||
|
gitea: g,
|
||||||
|
job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' }
|
||||||
|
});
|
||||||
|
assert.equal(out.diff, 'DIFF(c)\nDIFF(d)');
|
||||||
|
assert.equal(out.partial, false);
|
||||||
|
assert.deepEqual(g.calls.filter(c => c[0] === 'commit'), [['commit', 'o', 'r', C1], ['commit', 'o', 'r', C2]]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('API compare 404 too -> per-commit diffs from the webhook commitShas', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
const C1 = 'c'.repeat(40);
|
||||||
|
g.getCompareDiff = (o, r, before, after) =>
|
||||||
|
Promise.reject(nf(`/o/r/compare/${before}...${after}.diff`));
|
||||||
|
g.compare = (o, r, before, after) =>
|
||||||
|
Promise.reject(nf(`/api/v1/repos/o/r/compare/${before}...${after}`));
|
||||||
|
g.getCommitDiff = (o, r, sha) => {
|
||||||
|
g.calls.push(['commit', o, r, sha]);
|
||||||
|
return Promise.resolve(`DIFF(${sha.slice(0, 1)})`);
|
||||||
|
};
|
||||||
|
const out = await fetchDiff({
|
||||||
|
gitea: g,
|
||||||
|
job: {
|
||||||
|
owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE,
|
||||||
|
defaultBranch: 'main', commitShas: [C1, HEAD]
|
||||||
|
}
|
||||||
|
});
|
||||||
|
assert.equal(out.diff, 'DIFF(c)\nDIFF(a)');
|
||||||
|
assert.equal(out.partial, false);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('per-commit fallback stops fetching past maxBytes (review truncates after)', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
const shas = ['c'.repeat(40), 'd'.repeat(40), 'e'.repeat(40)];
|
||||||
|
g.getCompareDiff = () => Promise.reject(nf('/o/r/compare/x...y.diff'));
|
||||||
|
g.compare = () => Promise.resolve({ commits: shas.map(sha => ({ sha })) });
|
||||||
|
const fetched = [];
|
||||||
|
g.getCommitDiff = (o, r, sha) => { fetched.push(sha); return Promise.resolve('x'.repeat(10)); };
|
||||||
|
const out = await fetchDiff({
|
||||||
|
gitea: g,
|
||||||
|
job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' },
|
||||||
|
maxBytes: 15
|
||||||
|
});
|
||||||
|
// first two diffs exceed the cap (20 > 15) -> third commit never fetched
|
||||||
|
assert.equal(fetched.length, 2);
|
||||||
|
assert.equal(out.diff, 'x'.repeat(10) + '\n' + 'x'.repeat(10));
|
||||||
|
// byte-capped reconstruction covered 2/3 commits -> partial, verdict capped
|
||||||
|
assert.equal(out.partial, true);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('every source 404 -> throws listing every URL tried (then fail-open)', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
g.getCompareDiff = (o, r, before, after) =>
|
||||||
|
Promise.reject(nf(`/o/r/compare/${before}...${after}.diff`));
|
||||||
|
g.compare = (o, r, before, after) =>
|
||||||
|
Promise.reject(nf(`/api/v1/repos/o/r/compare/${before}...${after}`));
|
||||||
|
g.getCommitDiff = (o, r, sha) =>
|
||||||
|
Promise.reject(nf(`/api/v1/repos/o/r/git/commits/${sha}.diff`));
|
||||||
|
await assert.rejects(
|
||||||
|
() => fetchDiff({
|
||||||
|
gitea: g,
|
||||||
|
job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' }
|
||||||
|
}),
|
||||||
|
(e) => {
|
||||||
|
assert.match(e.message, /^all diff sources failed: /);
|
||||||
|
assert.match(e.message, new RegExp(`compare/${BEFORE}\\.\\.\\.${HEAD}\\.diff`));
|
||||||
|
assert.match(e.message, new RegExp(`compare/main\\.\\.\\.${HEAD}\\.diff`));
|
||||||
|
assert.match(e.message, new RegExp(`api/v1/repos/o/r/compare/${BEFORE}`));
|
||||||
|
assert.match(e.message, new RegExp(`git/commits/${HEAD}\\.diff`));
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('PR diff 404 falls back to the head commit diff', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
g.getPrDiff = (o, r, i) => Promise.reject(nf(`/api/v1/repos/o/r/pulls/${i}.diff`));
|
||||||
|
const out = await fetchDiff({ gitea: g, job: { owner: 'o', repo: 'r', prIndex: 7, sha: HEAD, before: null } });
|
||||||
|
assert.equal(out.diff, 'HEADDIFF');
|
||||||
|
// head-only PR fallback reviews a subset -> partial
|
||||||
|
assert.equal(out.partial, true);
|
||||||
|
assert.deepEqual(g.calls, [['commit', 'o', 'r', HEAD]]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('empty compare body is treated as a miss, not a reviewable diff', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
g.getCompareDiff = (o, r, before) => before === BEFORE ? Promise.resolve('') : Promise.resolve('RANGEDIFF');
|
||||||
|
const out = await fetchDiff({
|
||||||
|
gitea: g,
|
||||||
|
job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main' }
|
||||||
|
});
|
||||||
|
assert.equal(out.diff, 'RANGEDIFF');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('per-commit fallback dedupes, validates, and caps the sha list at 20', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
// 25 unique shas + 1 dup + 1 garbage entry: only the first 20 valid uniques fetch
|
||||||
|
const shas = Array.from({ length: 25 }, (_, i) => String(i).padStart(40, 'f'.charCodeAt ? '0' : '0'));
|
||||||
|
const payload = [...shas, shas[0], 'not-a-sha'];
|
||||||
|
g.getCompareDiff = () => Promise.reject(nf('/o/r/compare/x...y.diff'));
|
||||||
|
g.compare = () => Promise.reject(nf('/api/v1/repos/o/r/compare/x...y'));
|
||||||
|
const fetched = [];
|
||||||
|
g.getCommitDiff = (o, r, sha) => { fetched.push(sha); return Promise.resolve('D'); };
|
||||||
|
const out = await fetchDiff({
|
||||||
|
gitea: g,
|
||||||
|
job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE, defaultBranch: 'main', commitShas: payload }
|
||||||
|
});
|
||||||
|
assert.equal(fetched.length, 20);
|
||||||
|
assert.equal(out.partial, true); // 20/25 covered
|
||||||
|
});
|
||||||
|
|
||||||
|
test('getRepo auth failure propagates instead of being swallowed', async () => {
|
||||||
|
const g = giteaStub();
|
||||||
|
g.getCompareDiff = (o, r, before) => before === BEFORE
|
||||||
|
? Promise.reject(nf('/o/r/compare/x...y.diff'))
|
||||||
|
: Promise.resolve('RANGEDIFF');
|
||||||
|
const authErr = new Error('gitea GET /api/v1/repos/o/r -> 401: unauthorized');
|
||||||
|
authErr.status = 401;
|
||||||
|
g.getRepo = () => Promise.reject(authErr);
|
||||||
|
await assert.rejects(
|
||||||
|
() => fetchDiff({ gitea: g, job: { owner: 'o', repo: 'r', prIndex: null, sha: HEAD, before: BEFORE } }),
|
||||||
|
/401: unauthorized/
|
||||||
|
);
|
||||||
|
});
|
||||||
@@ -0,0 +1,25 @@
|
|||||||
|
import { test } from 'node:test';
|
||||||
|
import assert from 'node:assert/strict';
|
||||||
|
import { assertServedModelAllowed } from '../src/lib/review.js';
|
||||||
|
|
||||||
|
test('local model reviews accept the exact served local model', () => {
|
||||||
|
assert.doesNotThrow(() =>
|
||||||
|
assertServedModelAllowed('ollama-remote/qwen2.5-coder:7b', 'ollama-remote/qwen2.5-coder:7b'));
|
||||||
|
});
|
||||||
|
|
||||||
|
test('local model reviews fail on cloud fallback drift', () => {
|
||||||
|
assert.throws(
|
||||||
|
() => assertServedModelAllowed('ollama-remote/qwen2.5-coder:7b', 'google/gemini-2.5-flash'),
|
||||||
|
/local model contract violated/
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('cloud model reviews allow provider fallback reporting', () => {
|
||||||
|
assert.doesNotThrow(() =>
|
||||||
|
assertServedModelAllowed('anthropic/claude-sonnet-4-6', 'google/gemini-2.5-flash'));
|
||||||
|
});
|
||||||
|
|
||||||
|
test('missing served model metadata does not fail local requests', () => {
|
||||||
|
assert.doesNotThrow(() =>
|
||||||
|
assertServedModelAllowed('ollama-remote/qwen2.5-coder:7b', null));
|
||||||
|
});
|
||||||
@@ -0,0 +1,64 @@
|
|||||||
|
import { test } from 'node:test';
|
||||||
|
import assert from 'node:assert/strict';
|
||||||
|
import { openDb, enqueueJob, startJob, finishJob, recoverPendingJobs } from '../src/lib/db.js';
|
||||||
|
|
||||||
|
const JOB = (over = {}) => ({
|
||||||
|
event: 'push', owner: 'nirpa', repo: 'demo', sha: 'a'.repeat(40),
|
||||||
|
ref: 'refs/heads/main', before: 'b'.repeat(40), prIndex: null, prTitle: null,
|
||||||
|
pusher: 'nirpa', authors: ['nirpa'], commitShas: ['a'.repeat(40)],
|
||||||
|
commitMessages: ['msg'], ...over
|
||||||
|
});
|
||||||
|
|
||||||
|
test('enqueued jobs that never finish are recovered on startup', () => {
|
||||||
|
const db = openDb(':memory:');
|
||||||
|
const id1 = enqueueJob(db, JOB({ sha: '1'.repeat(40) }));
|
||||||
|
const id2 = enqueueJob(db, JOB({ sha: '2'.repeat(40) }));
|
||||||
|
const id3 = enqueueJob(db, JOB({ sha: '3'.repeat(40), prIndex: 7 }));
|
||||||
|
finishJob(db, id2, 'pass');
|
||||||
|
|
||||||
|
const pending = recoverPendingJobs(db);
|
||||||
|
assert.equal(pending.length, 2);
|
||||||
|
assert.deepEqual(pending.map(p => p.id), [id1, id3]);
|
||||||
|
// job payload round-trips intact
|
||||||
|
assert.equal(pending[0].job.sha, '1'.repeat(40));
|
||||||
|
assert.equal(pending[1].job.prIndex, 7);
|
||||||
|
assert.deepEqual(pending[0].job.commitMessages, ['msg']);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('finished jobs are not recovered again', () => {
|
||||||
|
const db = openDb(':memory:');
|
||||||
|
const id = enqueueJob(db, JOB());
|
||||||
|
finishJob(db, id, 'warn');
|
||||||
|
assert.equal(recoverPendingJobs(db).length, 0);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('running jobs are visible and recovered on startup', () => {
|
||||||
|
const db = openDb(':memory:');
|
||||||
|
const id = enqueueJob(db, JOB({ sha: '4'.repeat(40) }));
|
||||||
|
startJob(db, id);
|
||||||
|
|
||||||
|
const row = db.prepare(`SELECT state, started_ts FROM queue_jobs WHERE id = ?`).get(id);
|
||||||
|
assert.equal(row.state, 'running');
|
||||||
|
assert.match(row.started_ts, /^\d{4}-\d{2}-\d{2}T/);
|
||||||
|
|
||||||
|
const pending = recoverPendingJobs(db);
|
||||||
|
assert.deepEqual(pending.map(p => p.id), [id]);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('unparseable persisted job is marked done, not re-run forever', () => {
|
||||||
|
const db = openDb(':memory:');
|
||||||
|
db.prepare(`INSERT INTO queue_jobs (repo, sha, job) VALUES ('a/b', 'c', '{broken')`).run();
|
||||||
|
assert.equal(recoverPendingJobs(db).length, 0);
|
||||||
|
// and it is now terminal
|
||||||
|
assert.equal(recoverPendingJobs(db).length, 0);
|
||||||
|
const row = db.prepare(`SELECT state, outcome FROM queue_jobs`).get();
|
||||||
|
assert.equal(row.state, 'done');
|
||||||
|
assert.equal(row.outcome, 'unparseable-job');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('recovery is ordered oldest first', () => {
|
||||||
|
const db = openDb(':memory:');
|
||||||
|
const ids = [enqueueJob(db, JOB()), enqueueJob(db, JOB()), enqueueJob(db, JOB())];
|
||||||
|
const pending = recoverPendingJobs(db);
|
||||||
|
assert.deepEqual(pending.map(p => p.id), ids);
|
||||||
|
});
|
||||||
@@ -0,0 +1,57 @@
|
|||||||
|
import { test } from 'node:test';
|
||||||
|
import assert from 'node:assert/strict';
|
||||||
|
|
||||||
|
import { ReviewQueue } from '../src/lib/queue.js';
|
||||||
|
|
||||||
|
test('newer jobs supersede queued work for the same pull request', async () => {
|
||||||
|
const queue = new ReviewQueue();
|
||||||
|
const ran = [];
|
||||||
|
const superseded = [];
|
||||||
|
|
||||||
|
const first = queue.enqueue(
|
||||||
|
'nirpa/hermes-agent',
|
||||||
|
async () => ran.push('old'),
|
||||||
|
{ dedupeKey: 'pr:86', onSuperseded: () => superseded.push('old') }
|
||||||
|
);
|
||||||
|
const second = queue.enqueue(
|
||||||
|
'nirpa/hermes-agent',
|
||||||
|
async () => ran.push('new'),
|
||||||
|
{ dedupeKey: 'pr:86', onSuperseded: () => superseded.push('new') }
|
||||||
|
);
|
||||||
|
|
||||||
|
await Promise.all([first, second]);
|
||||||
|
assert.deepEqual(ran, ['new']);
|
||||||
|
assert.deepEqual(superseded, ['old']);
|
||||||
|
assert.equal(queue.pending, 0);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('different pull requests are not deduplicated', async () => {
|
||||||
|
const queue = new ReviewQueue();
|
||||||
|
const ran = [];
|
||||||
|
|
||||||
|
await Promise.all([
|
||||||
|
queue.enqueue('nirpa/hermes-agent', async () => ran.push('pr-86'), { dedupeKey: 'pr:86' }),
|
||||||
|
queue.enqueue('nirpa/hermes-agent', async () => ran.push('pr-87'), { dedupeKey: 'pr:87' })
|
||||||
|
]);
|
||||||
|
|
||||||
|
assert.deepEqual(ran, ['pr-86', 'pr-87']);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('shared sha key deduplicates push and pull-request webhooks', async () => {
|
||||||
|
const queue = new ReviewQueue();
|
||||||
|
const ran = [];
|
||||||
|
|
||||||
|
const push = queue.enqueue(
|
||||||
|
'nirpa/hermes-agent',
|
||||||
|
async () => ran.push('push'),
|
||||||
|
{ dedupeKeys: ['sha:final'] }
|
||||||
|
);
|
||||||
|
const pullRequest = queue.enqueue(
|
||||||
|
'nirpa/hermes-agent',
|
||||||
|
async () => ran.push('pull-request'),
|
||||||
|
{ dedupeKeys: ['pr:86', 'sha:final'] }
|
||||||
|
);
|
||||||
|
|
||||||
|
await Promise.all([push, pullRequest]);
|
||||||
|
assert.deepEqual(ran, ['pull-request']);
|
||||||
|
});
|
||||||
@@ -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/);
|
||||||
|
});
|
||||||
@@ -0,0 +1,106 @@
|
|||||||
|
import { test } from 'node:test';
|
||||||
|
import assert from 'node:assert/strict';
|
||||||
|
import { chmod, mkdtemp, writeFile } from 'node:fs/promises';
|
||||||
|
import { tmpdir } from 'node:os';
|
||||||
|
import { join } from 'node:path';
|
||||||
|
import { chat } from '../src/lib/router.js';
|
||||||
|
|
||||||
|
test('review router client disables model fallbacks explicitly', async () => {
|
||||||
|
const originalFetch = globalThis.fetch;
|
||||||
|
let body;
|
||||||
|
globalThis.fetch = async (_url, init) => {
|
||||||
|
body = JSON.parse(init.body);
|
||||||
|
return Response.json({
|
||||||
|
message: { content: '{"axes":{},"overall":90,"grade":"A","findings":[]}' },
|
||||||
|
_shre: { model: 'ollama-remote/qwen2.5-coder:7b' }
|
||||||
|
});
|
||||||
|
};
|
||||||
|
|
||||||
|
try {
|
||||||
|
await chat({
|
||||||
|
routerUrl: 'http://router.test',
|
||||||
|
model: 'ollama-remote/qwen2.5-coder:7b',
|
||||||
|
messages: [{ role: 'user', content: 'review' }],
|
||||||
|
agentId: 'code-reviewer',
|
||||||
|
sessionId: 'review-test',
|
||||||
|
tenantId: 'nirlab',
|
||||||
|
timeoutMs: 1000
|
||||||
|
});
|
||||||
|
} finally {
|
||||||
|
globalThis.fetch = originalFetch;
|
||||||
|
}
|
||||||
|
|
||||||
|
assert.deepEqual(body.fallbackModels, []);
|
||||||
|
assert.equal(body.tools, false);
|
||||||
|
assert.equal(body.raw, true);
|
||||||
|
assert.equal(body.stream, false);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('claude-cli models bypass the router and use subscription credentials', async () => {
|
||||||
|
const dir = await mkdtemp(join(tmpdir(), 'granthi-review-claude-'));
|
||||||
|
const fakeClaude = join(dir, 'claude');
|
||||||
|
await writeFile(fakeClaude, `#!/bin/sh
|
||||||
|
case " $* " in
|
||||||
|
*" --model claude-sonnet-4-6 "*) ;;
|
||||||
|
*) exit 21 ;;
|
||||||
|
esac
|
||||||
|
case " $* " in
|
||||||
|
*" --tools --permission-mode dontAsk --no-session-persistence "*) ;;
|
||||||
|
*) exit 22 ;;
|
||||||
|
esac
|
||||||
|
[ -z "\${ANTHROPIC_API_KEY:-}" ] || exit 23
|
||||||
|
[ -z "\${ANTHROPIC_TOKEN:-}" ] || exit 24
|
||||||
|
printf '%s' '{"result":"{\\"axes\\":{},\\"overall\\":90,\\"grade\\":\\"A\\",\\"findings\\":[]}"}'
|
||||||
|
`);
|
||||||
|
await chmod(fakeClaude, 0o700);
|
||||||
|
|
||||||
|
const originalFetch = globalThis.fetch;
|
||||||
|
const originalApiKey = process.env.ANTHROPIC_API_KEY;
|
||||||
|
const originalToken = process.env.ANTHROPIC_TOKEN;
|
||||||
|
globalThis.fetch = async () => { throw new Error('router must not be called'); };
|
||||||
|
process.env.ANTHROPIC_API_KEY = 'test-api-key';
|
||||||
|
process.env.ANTHROPIC_TOKEN = 'test-token';
|
||||||
|
|
||||||
|
try {
|
||||||
|
const result = await chat({
|
||||||
|
routerUrl: 'http://router.test',
|
||||||
|
model: 'claude-cli/claude-sonnet-4-6',
|
||||||
|
messages: [{ role: 'user', content: 'review this diff' }],
|
||||||
|
agentId: 'code-reviewer',
|
||||||
|
sessionId: 'review-test',
|
||||||
|
tenantId: 'nirlab',
|
||||||
|
timeoutMs: 1000,
|
||||||
|
claudeBin: fakeClaude
|
||||||
|
});
|
||||||
|
assert.match(result.content, /"overall":90/);
|
||||||
|
assert.equal(result.servedModel, 'claude-cli/claude-sonnet-4-6');
|
||||||
|
} finally {
|
||||||
|
globalThis.fetch = originalFetch;
|
||||||
|
if (originalApiKey === undefined) delete process.env.ANTHROPIC_API_KEY;
|
||||||
|
else process.env.ANTHROPIC_API_KEY = originalApiKey;
|
||||||
|
if (originalToken === undefined) delete process.env.ANTHROPIC_TOKEN;
|
||||||
|
else process.env.ANTHROPIC_TOKEN = originalToken;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('claude-cli failures do not leak the review prompt', async () => {
|
||||||
|
const dir = await mkdtemp(join(tmpdir(), 'granthi-review-claude-error-'));
|
||||||
|
const fakeClaude = join(dir, 'claude');
|
||||||
|
await writeFile(fakeClaude, '#!/bin/sh\nprintf "%s\\n" "$*" >&2\nexit 25\n');
|
||||||
|
await chmod(fakeClaude, 0o700);
|
||||||
|
|
||||||
|
await assert.rejects(
|
||||||
|
chat({
|
||||||
|
routerUrl: 'http://router.test',
|
||||||
|
model: 'claude-cli/claude-sonnet-4-6',
|
||||||
|
messages: [{ role: 'user', content: 'SECRET_REVIEW_DIFF_MARKER' }],
|
||||||
|
timeoutMs: 1000,
|
||||||
|
claudeBin: fakeClaude
|
||||||
|
}),
|
||||||
|
(error) => {
|
||||||
|
assert.match(error.message, /claude-cli failed/);
|
||||||
|
assert.doesNotMatch(error.message, /SECRET_REVIEW_DIFF_MARKER/);
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
);
|
||||||
|
});
|
||||||
@@ -78,3 +78,30 @@ test('normalizeReview tolerates junk', () => {
|
|||||||
assert.equal(r.verdict, 'pass');
|
assert.equal(r.verdict, 'pass');
|
||||||
assert.equal(r.findings.length, 0);
|
assert.equal(r.findings.length, 0);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// --- applyTruncationCap ---
|
||||||
|
import { applyTruncationCap } from '../src/lib/verdict.js';
|
||||||
|
|
||||||
|
test('truncation caps pass at warn', () => {
|
||||||
|
const r = { verdict: 'pass' };
|
||||||
|
applyTruncationCap(r, true);
|
||||||
|
assert.equal(r.verdict, 'warn');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('truncation leaves warn as warn', () => {
|
||||||
|
const r = { verdict: 'warn' };
|
||||||
|
applyTruncationCap(r, true);
|
||||||
|
assert.equal(r.verdict, 'warn');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('truncation does not downgrade fail', () => {
|
||||||
|
const r = { verdict: 'fail' };
|
||||||
|
applyTruncationCap(r, true);
|
||||||
|
assert.equal(r.verdict, 'fail');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('no truncation leaves pass untouched', () => {
|
||||||
|
const r = { verdict: 'pass' };
|
||||||
|
applyTruncationCap(r, false);
|
||||||
|
assert.equal(r.verdict, 'pass');
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user