mirror of
https://github.com/expo/code-review-cli.git
synced 2026-11-15 03:38:09 -10:00
Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
813859b6cf |
No files matched your search
@@ -0,0 +1,230 @@
|
||||
# Plan: inline PR comments for in-diff findings (rev 2, post adversarial review)
|
||||
|
||||
## Goal
|
||||
|
||||
Findings anchored to a line in the PR diff land as inline review comments on
|
||||
that line. The main issue comment keeps every finding but renders an inlined
|
||||
one SHORT: title + one-line rationale + link to the inline thread. The main
|
||||
comment stays the single durable state store (embedded state, dismissals,
|
||||
feedback records, pins, `/dismiss` target). Inline comments are a derived
|
||||
presentation layer, converged on every run.
|
||||
|
||||
## Non-goals (v1)
|
||||
|
||||
- No formal `POST /pulls/{n}/reviews` review event: a batched review is atomic
|
||||
(one bad anchor 422s the whole post) and cannot be upserted across runs.
|
||||
Individual review comments isolate failures and PATCH in place.
|
||||
- No multi-line comments; single `line` + `side: "RIGHT"` only.
|
||||
- No GraphQL thread resolution, and no detection of author-resolved/outdated
|
||||
threads. Accepted risk: the PR author can collapse inline threads; the main
|
||||
comment therefore keeps a one-line rationale per finding (see Short form).
|
||||
- `ecr feedback` retroactive crawl keeps reading issue comments only. The
|
||||
review-comments fetch in `replyComments` is gated on the reporter having an
|
||||
ENABLED `inline` option, so the crawl (which passes none) gains no fetches.
|
||||
- Scopes removed from the routing manifest strand their comments — same
|
||||
pre-existing behavior as their main comments.
|
||||
- Break-glass (`postSkipNote`) leaves inline threads untouched even though it
|
||||
overwrites the embedded state. Accepted, documented.
|
||||
|
||||
## Config
|
||||
|
||||
Root-only key in `ReviewConfigSchema`, locked out of scope configs with
|
||||
`z.never` like `feedback`:
|
||||
|
||||
```jsonc
|
||||
inline: {
|
||||
enabled: false, // ship dark
|
||||
maxComments: 20 // cap per posted comment's reporter (per PR in single/
|
||||
// legacy mode; per scope in per-scope mode — documented)
|
||||
}
|
||||
```
|
||||
|
||||
Wired into `LoadedConfig`, `load.ts` defaults, all five reporter construction
|
||||
sites, and `reviewPostingConfigFingerprint` (posting behavior changes with it,
|
||||
so a saved deferred review must not post under a different inline policy).
|
||||
Turning it OFF later leaves existing threads up (documented; `clear()` still
|
||||
sweeps them on mode switches, and a manual cleanup is possible).
|
||||
|
||||
## Inline comment identity
|
||||
|
||||
Marker, at byte 0 of the body, parsed with an ANCHORED regex and a hex-only
|
||||
charset:
|
||||
|
||||
```
|
||||
<!-- <tag>:inline:fp=<[a-f0-9]{6,64}> -->
|
||||
```
|
||||
|
||||
- Identity = author (`user.login === ownLogin`) + anchored marker; marker alone
|
||||
is never identity (selectOwnComments model). ownLogin unresolvable ⇒ nothing
|
||||
is ours AND no creates either (fail closed, no duplicate storms).
|
||||
- Byte-0 anchoring also narrows the shared `github-actions[bot]` confused-deputy
|
||||
risk: an echo-bot that prefixes any content fails the anchor.
|
||||
- fp is the id the owning main comment renders (plain, or scope-namespaced for
|
||||
the aggregate). Substring safety vs `<!-- tag -->` / `<!-- tag:scope -->`
|
||||
holds in both directions (closing ` -->` differs), incl. a scope named
|
||||
`inline`.
|
||||
|
||||
## Inline comment body
|
||||
|
||||
Marker line, then the finding rendered in full (severity, title, category,
|
||||
`id:<fp>` token, rationale, sources, suggestion — every model-written field
|
||||
through `stripStateMarkers`), then a footer: "Reply here to respond; write
|
||||
`id:<fp>` in your reply (unquoted) to formally answer it."
|
||||
|
||||
**Self-reply backstop (was blocker):** the body carries an unquoted `id:` token,
|
||||
and today's `OWN_COMMENT_RE` (`:state=|:fingerprints=`) would NOT exclude it if
|
||||
identity/tag filters ever miss (bot-login fallback, commentTag rename, second
|
||||
posting identity). Fixes, all three:
|
||||
- `responses.ts` gains a tag-independent backstop: a body whose UNQUOTED lines
|
||||
contain `:inline:fp=` is never treated as a reply (quoted lines excluded so a
|
||||
GitHub "Quote reply" of our inline comment — which copies the marker behind
|
||||
`> ` — still counts as a genuine reply).
|
||||
- `replyComments` applies the same unquoted-line rule for the tag-specific
|
||||
marker.
|
||||
- Test: an inline body is never matched as a reply even with `ownLogin: null`
|
||||
and a foreign tag; a quote-reply containing the quoted marker IS matched.
|
||||
|
||||
## Reporter mechanics
|
||||
|
||||
New option `inline?: LoadedConfig["inline"]`.
|
||||
|
||||
### Fetch layer
|
||||
|
||||
`fetchAllReviewComments()`: paginated GET `repos/{repo}/pulls/{n}/comments`
|
||||
(fields: id, body, path, line, user.login, html_url, in_reply_to_id,
|
||||
author_association). Cached in a MODULE-LEVEL map keyed
|
||||
`prNumber\nrepo\ncwd` (the prAuthorByPr pattern) with the same 30s TTL and
|
||||
invalidate-on-mutation — routed single-mode runs call `clear()` on a fresh
|
||||
reporter per scope, and per-instance caching would multiply listings. Note:
|
||||
this endpoint DOES honor sort/direction (unlike issue comments) — do not copy
|
||||
that rationale comment; a full scan is still needed.
|
||||
|
||||
### Sync
|
||||
|
||||
`syncInline(targets, link, opts: { teardown: boolean })` runs inside
|
||||
`report()` / `reportAggregate()` before rendering; returns `Map<fp, url>`.
|
||||
|
||||
Targets = ACTIVE findings only (not dismissed, not reply-cleared, not
|
||||
requalified) with `line != null` and `diffLines.get(file).has(line)`.
|
||||
|
||||
**Cap (sticky, was churn bug):** order candidates by severity rank then fp
|
||||
(deterministic); fill slots preferring fps that ALREADY have a live inline
|
||||
comment, then admit new ones. A live thread is never evicted to create a new
|
||||
one.
|
||||
|
||||
**Teardown gating (was blocker):** stubs/deletes run ONLY when
|
||||
`opts.teardown` is true, which callers set as:
|
||||
- `report()`: `!review.couldNotComplete`.
|
||||
- `reportAggregate()`: caller-passed flag — ci.ts passes
|
||||
`!scopesFilter && results.every(r => !r.review.couldNotComplete)`. A partial
|
||||
`--scopes` run or any failed scope makes the run additive-only (creates +
|
||||
patches, no stubs/deletes), because carried/truncated/failed state cannot
|
||||
prove a finding is gone. (`scopedFingerprint` is opaque hex — a per-scope
|
||||
stale set cannot be derived from fps, so the gate is all-or-nothing.)
|
||||
- `applyDismissal`: never computes a stale set. Additive-only: stub threads of
|
||||
the explicitly dismissed fps; on `/undismiss`, re-patch the full body if the
|
||||
finding is present in state (skip if truncated away).
|
||||
- Prereq fix: both failure literals (`ci.ts` legacy catch and
|
||||
`failureReview()`) gain `couldNotComplete: true` — today they post
|
||||
empty-findings reviews WITHOUT the flag and would tear everything down. Side
|
||||
effects checked: the decision label becomes the accurate "No review — every
|
||||
pass failed", and caching is unaffected (failures already never carry
|
||||
inputHash).
|
||||
|
||||
**Stub text (was blocker):** neutral — "This finding is no longer tracked
|
||||
inline — see the main review comment for current status." Never
|
||||
"resolved/dismissed": a capped-out or drifted finding is still active.
|
||||
|
||||
**Mutations** (sequential, never parallel):
|
||||
- create: POST with body/commit_id=headSha/path/line/side. Creates require
|
||||
headSha AND a live-head check: one `gh pr view --json headRefOid` per sync
|
||||
(only when creates exist); mismatch ⇒ skip creates, log (a mid-review push
|
||||
means our diff no longer matches the head; a stale commit_id can silently
|
||||
anchor to wrong code, and a force-push 422s every create).
|
||||
- patch: only when the normalized body differs (`\r\n` → `\n`) — no PATCH-per-
|
||||
finding-per-run forever.
|
||||
- stale (teardown only): thread has replies ⇒ PATCH to the neutral stub
|
||||
(marker kept, so a returning finding revives the thread); no replies ⇒
|
||||
DELETE. Reply detection: any listed comment with `in_reply_to_id` == our id
|
||||
(same snapshot; the reply-between-list-and-delete race is accepted, noted in
|
||||
tests).
|
||||
- duplicates (crash window): keep newest per fp, older are stale.
|
||||
- 422 on create ⇒ log once + skip that finding (renders full-form). 403/429
|
||||
(secondary rate limit) ⇒ log + abort ALL remaining mutations this run.
|
||||
- Audit (was silent): stderr one-liner + step-summary line:
|
||||
`inline sync: N created, N patched, N stubbed, N deleted, N failed`.
|
||||
|
||||
Every mutation individually try/caught; the inline layer never blocks the main
|
||||
comment. Listing failure ⇒ empty map, no mutations.
|
||||
|
||||
### clear()
|
||||
|
||||
Extends to inline comments under this reporter's tag, gated on marker+author
|
||||
only (NOT on `enabled`, so mode switches clean up after a disable): replied
|
||||
threads are STUBBED, bare ones deleted — never delete a thread humans wrote in
|
||||
(ci.ts calls clear() on mode switches and scope reconciliation). This also
|
||||
covers the fp-namespace flip on per-scope↔single switches, symmetric with how
|
||||
main comments are cleared today.
|
||||
|
||||
### Main-comment short form
|
||||
|
||||
- `LinkContext.inlineUrls?: Map<string, string>`. NEVER mutate the shared
|
||||
routed-flow `link` object — render with a copy `{ ...link, inlineUrls }`.
|
||||
- `renderFindingLines` with an inline URL: `- **title** —
|
||||
[file:line](thread-url) _(category)_ · id:… · 💬 [inline](url)` + reply
|
||||
annotation + ONE-LINE truncated rationale (~160 chars, word boundary) — the
|
||||
audit trail survives the author collapsing/resolving inline threads.
|
||||
Sources/suggestion/full rationale live inline only. Missing URL ⇒ full form
|
||||
(tested).
|
||||
- Embedded state unchanged.
|
||||
|
||||
### Inline replies → feedback
|
||||
|
||||
- `replyComments()` maps review comments too — ONLY when
|
||||
`options.inline?.enabled` (keeps `ecr feedback` crawl and non-inline repos at
|
||||
zero extra fetches). Excludes own login; excludes bodies whose unquoted lines
|
||||
carry our markers.
|
||||
- **Id namespace (was collision risk):** review-comment-sourced ReplyComments
|
||||
get `id = -reviewCommentId`. Negative ids flow into FeedbackRecord.commentId
|
||||
(schema unchanged — number stays a number) and are consistent across runs, so
|
||||
mergeFeedback's `commentId` equality and buildAdjudicationItems' body lookup
|
||||
stay collision-free vs the positive issue-comment space.
|
||||
- `applyPins` gains a cross-stream guard: a pin is lifted only by a maintainer
|
||||
record whose commentId is newer AND in the SAME stream (same sign). A
|
||||
different-stream comparison is undecidable ordering ⇒ keep the pin (fail
|
||||
closed).
|
||||
- `ReplyComment.threadFp?: string`: set when the reply's `in_reply_to_id`
|
||||
resolves to OUR (author+anchored-marker) top-level inline comment's fp.
|
||||
`matchReplies` treats a known threadFp as a match in EVERY `match` mode
|
||||
(structural signal, not text matching; documented + tested). `citedId` is
|
||||
unchanged: clearing still requires the replier's own unquoted `id:` token.
|
||||
- Newest-wins across streams: negative ids lose to positive ones by raw
|
||||
comparison; acceptable because only the newest record per fp matters and a
|
||||
same-fp cross-stream tie is rare; the applyPins guard removes the dangerous
|
||||
consequence.
|
||||
|
||||
## Files touched
|
||||
|
||||
- `src/config/schema.ts`, `src/config/load.ts` — `inline` key + lock + defaults.
|
||||
- `src/core/deferred-review.ts` — fingerprint includes `inline`.
|
||||
- `src/reporters/github.ts` — review-comments fetch (shared cache), syncInline
|
||||
+ pure exported planner, replyComments extension, clear(), applyDismissal
|
||||
additive sync.
|
||||
- `src/core/render.ts` — inline marker helpers (anchored parse), inline body
|
||||
renderer, `LinkContext.inlineUrls`, short form + rationale truncation.
|
||||
- `src/core/responses.ts` — threadFp, unquoted-marker backstop.
|
||||
- `src/core/schema.ts` — applyPins same-stream guard.
|
||||
- `src/commands/ci.ts` — wire config, `couldNotComplete: true` on both failure
|
||||
literals, aggregate teardown flag. `review.ts`, `post-review.ts`,
|
||||
`dismiss.ts` — wire config.
|
||||
- Tests: planner (create/patch/stub/delete, sticky cap, dedupe, additive mode),
|
||||
marker anchor/forgery, short form + missing-URL fallback, backstop vs
|
||||
quote-reply, threadFp in all match modes, applyPins stream guard,
|
||||
couldNotComplete literals.
|
||||
- `templates/config.jsonc`, `README.md`.
|
||||
|
||||
## Rollout
|
||||
|
||||
Ship dark. Note in README: first enable on a large PR fires one notification
|
||||
per created inline comment (cap bounds it). Enable on this repo first, then
|
||||
expo/expo after one observed cycle.
|
||||
@@ -651,6 +651,44 @@ the defaults are asymmetric: [LLP 0011](./llp/0011-author-feedback.explainer.md)
|
||||
|
||||
</details>
|
||||
|
||||
<details>
|
||||
<summary><b>Inline PR comments (findings on the diff line)</b></summary>
|
||||
|
||||
Off by default. When enabled, each finding anchored to a line that is actually in
|
||||
the PR's diff is posted as an inline review comment on that line, carrying the
|
||||
full rationale, sources, and suggestion. The main comment still lists every
|
||||
finding, but an inlined one renders short — title, links, category, id, and a
|
||||
one-line rationale — plus a `💬 inline comment` link to the thread. The main
|
||||
comment remains the single durable state store: `/dismiss`, embedded state, and
|
||||
reply adjudication are unchanged.
|
||||
|
||||
```jsonc
|
||||
"inline": {
|
||||
"enabled": false,
|
||||
"maxComments": 20 // per PR (single comment mode) / per scope (per-scope mode)
|
||||
}
|
||||
```
|
||||
|
||||
Behavior notes:
|
||||
|
||||
- **Threads converge, never churn.** Each inline comment carries a per-finding
|
||||
marker and is patched in place across re-reviews. The cap is sticky: a live
|
||||
thread is never evicted to create a new one. A finding that leaves the set is
|
||||
stubbed if humans replied in its thread (never deleted), deleted only when bare.
|
||||
- **Fail-safe teardown.** A failed run, a `--scopes` partial run, or a truncated
|
||||
aggregate never tears threads down — teardown only runs when the target set is
|
||||
provably complete. Creates are skipped (and logged) when the PR head moved
|
||||
during the review.
|
||||
- **Replies on inline threads count as author feedback.** Replying in a thread
|
||||
matches the reply to that finding structurally; clearing still requires the
|
||||
`id:…` token in the replier's own words, same as main-comment replies.
|
||||
- **Notification fan-out.** The first enabled run on a PR sends one notification
|
||||
per created inline comment; `maxComments` bounds it.
|
||||
- **Disabling later** leaves existing threads up (the sync no longer runs); a
|
||||
comment-mode switch — or the reporter's clear path — stubs/removes them.
|
||||
|
||||
</details>
|
||||
|
||||
<details>
|
||||
<summary><b>External context (--context-file)</b></summary>
|
||||
|
||||
|
||||
@@ -0,0 +1,369 @@
|
||||
import { test, expect } from "bun:test";
|
||||
|
||||
import {
|
||||
activeFindings,
|
||||
hasUnquotedMarker,
|
||||
inlineBodiesEqual,
|
||||
planInlineSync,
|
||||
} from "../reporters/github.js";
|
||||
import type { ExistingInlineComment } from "../reporters/github.js";
|
||||
import {
|
||||
inlineCommentMarker,
|
||||
inlineStubBody,
|
||||
oneLineRationale,
|
||||
parseInlineMarkerFp,
|
||||
renderInlineCommentBody,
|
||||
renderMarkdown,
|
||||
} from "../core/render.js";
|
||||
import type { LinkContext } from "../core/render.js";
|
||||
import { hasUnquotedInlineMarker, matchReplies, replyIsNewer } from "../core/responses.js";
|
||||
import type { ReplyComment } from "../core/responses.js";
|
||||
import { applyPins, fingerprintFinding } from "../core/schema.js";
|
||||
import type { CoordinatorOutput, FeedbackRecord, Finding } from "../core/schema.js";
|
||||
|
||||
const finding = (over: Partial<Finding> = {}): Finding => ({
|
||||
severity: "warning",
|
||||
category: "quality",
|
||||
file: "a.ts",
|
||||
line: 5,
|
||||
title: "Observer restarts for cached instances",
|
||||
rationale: "getInstances() restarts the push token observer for cached instances.",
|
||||
...over,
|
||||
});
|
||||
|
||||
const existing = (over: Partial<ExistingInlineComment> = {}): ExistingInlineComment => ({
|
||||
id: 1,
|
||||
fp: "aaaa11",
|
||||
body: "old",
|
||||
hasReplies: false,
|
||||
...over,
|
||||
});
|
||||
|
||||
const TAG = "expo-ai-code-reviewer";
|
||||
|
||||
// ---- marker format + anchored parse ----
|
||||
|
||||
test("inline marker roundtrips through the anchored parser", () => {
|
||||
const body = renderInlineCommentBody(finding(), TAG, "deadbeef11");
|
||||
expect(body.startsWith(inlineCommentMarker(TAG, "deadbeef11"))).toBe(true);
|
||||
expect(parseInlineMarkerFp(body, TAG)).toBe("deadbeef11");
|
||||
expect(parseInlineMarkerFp(inlineStubBody(TAG, "deadbeef11"), TAG)).toBe("deadbeef11");
|
||||
});
|
||||
|
||||
test("parseInlineMarkerFp: a marker anywhere but byte 0 parses to nothing", () => {
|
||||
const forged = `prose first\n${inlineCommentMarker(TAG, "deadbeef11")}`;
|
||||
expect(parseInlineMarkerFp(forged, TAG)).toBeNull();
|
||||
// A quote-reply carrying the marker behind `> ` is not identity either.
|
||||
expect(parseInlineMarkerFp(`> ${inlineCommentMarker(TAG, "deadbeef11")}`, TAG)).toBeNull();
|
||||
});
|
||||
|
||||
test("parseInlineMarkerFp: constrains the fp to the fingerprint alphabet and the exact tag", () => {
|
||||
expect(parseInlineMarkerFp(`<!-- ${TAG}:inline:fp=NOTHEX -->`, TAG)).toBeNull();
|
||||
expect(parseInlineMarkerFp(`<!-- other-tag:inline:fp=deadbeef11 -->`, TAG)).toBeNull();
|
||||
// A scoped tag never cross-parses under the root tag.
|
||||
expect(parseInlineMarkerFp(`<!-- ${TAG}:scope:inline:fp=deadbeef11 -->`, TAG)).toBeNull();
|
||||
});
|
||||
|
||||
// ---- inline body ----
|
||||
|
||||
test("renderInlineCommentBody: id token, severity, rationale, suggestion, and no state markers", () => {
|
||||
const body = renderInlineCommentBody(
|
||||
finding({ suggestion: "cache it", rationale: "watch <!-- evil --> out" }),
|
||||
TAG,
|
||||
"deadbeef11",
|
||||
);
|
||||
expect(body).toContain("`id:deadbeef11`");
|
||||
expect(body).toContain("🟡 Warning");
|
||||
expect(body).toContain("**Suggestion:** cache it");
|
||||
// Untrusted text can never smuggle a raw HTML comment opener into our body.
|
||||
expect(body).toContain("<!-- evil -->");
|
||||
expect(body).not.toMatch(/:(?:state|fingerprints)=/);
|
||||
});
|
||||
|
||||
test("inlineStubBody: neutral — never claims resolved or dismissed", () => {
|
||||
const stub = inlineStubBody(TAG, "deadbeef11");
|
||||
expect(stub.toLowerCase()).not.toContain("resolved");
|
||||
expect(stub.toLowerCase()).not.toContain("dismissed");
|
||||
expect(stub).toContain("main review comment");
|
||||
});
|
||||
|
||||
// ---- oneLineRationale ----
|
||||
|
||||
test("oneLineRationale: collapses whitespace, escapes <, truncates at a word boundary", () => {
|
||||
expect(oneLineRationale("a\n\nb c")).toBe("a b c");
|
||||
expect(oneLineRationale("<details>x</details>")).toBe("<details>x</details>");
|
||||
const long = Array.from({ length: 60 }, (_, i) => `word${i}`).join(" ");
|
||||
const short = oneLineRationale(long);
|
||||
expect(short.length).toBeLessThanOrEqual(161);
|
||||
expect(short.endsWith("…")).toBe(true);
|
||||
// Cut lands on a word boundary: the kept prefix is followed by a space in the input.
|
||||
const kept = short.slice(0, -1);
|
||||
expect(long.startsWith(kept)).toBe(true);
|
||||
expect(long[kept.length]).toBe(" ");
|
||||
});
|
||||
|
||||
// ---- planner ----
|
||||
|
||||
test("planInlineSync: creates new, patches live, stubs replied stale, deletes bare stale", () => {
|
||||
const target = { fp: "aaaa11", finding: finding() };
|
||||
const plan = planInlineSync({
|
||||
targets: [target, { fp: "bbbb22", finding: finding({ file: "b.ts" }) }],
|
||||
existing: [
|
||||
existing({ id: 10, fp: "aaaa11" }),
|
||||
existing({ id: 11, fp: "gone01", hasReplies: true }),
|
||||
existing({ id: 12, fp: "gone02", hasReplies: false }),
|
||||
],
|
||||
maxComments: 20,
|
||||
teardown: true,
|
||||
});
|
||||
expect(plan.patch.map((p) => p.comment.fp)).toEqual(["aaaa11"]);
|
||||
expect(plan.create.map((c) => c.fp)).toEqual(["bbbb22"]);
|
||||
expect(plan.stub.map((s) => s.fp)).toEqual(["gone01"]);
|
||||
expect(plan.remove.map((r) => r.fp)).toEqual(["gone02"]);
|
||||
});
|
||||
|
||||
test("planInlineSync: teardown=false is additive-only, but still removes duplicates of a kept fp", () => {
|
||||
const plan = planInlineSync({
|
||||
targets: [{ fp: "aaaa11", finding: finding() }],
|
||||
existing: [
|
||||
existing({ id: 10, fp: "aaaa11" }),
|
||||
existing({ id: 9, fp: "aaaa11" }), // older duplicate (crash window)
|
||||
existing({ id: 11, fp: "gone01" }),
|
||||
existing({ id: 12, fp: "gone02", hasReplies: true }),
|
||||
],
|
||||
maxComments: 20,
|
||||
teardown: false,
|
||||
});
|
||||
expect(plan.stub).toEqual([]);
|
||||
expect(plan.remove.map((r) => r.id)).toEqual([9]); // duplicate only, never live stale threads
|
||||
expect(plan.patch.map((p) => p.comment.id)).toEqual([10]); // newest duplicate is the live one
|
||||
});
|
||||
|
||||
test("planInlineSync: the cap is sticky — a live thread is never evicted for a new finding", () => {
|
||||
const critical = { fp: "cccc33", finding: finding({ severity: "critical", file: "c.ts" }) };
|
||||
const existingWarning = { fp: "aaaa11", finding: finding() };
|
||||
const plan = planInlineSync({
|
||||
targets: [critical, existingWarning],
|
||||
existing: [existing({ id: 10, fp: "aaaa11", hasReplies: true })],
|
||||
maxComments: 1,
|
||||
teardown: true,
|
||||
});
|
||||
// The warning already has a live thread; the (higher-severity) newcomer waits.
|
||||
expect(plan.patch.map((p) => p.comment.fp)).toEqual(["aaaa11"]);
|
||||
expect(plan.create).toEqual([]);
|
||||
expect(plan.stub).toEqual([]);
|
||||
});
|
||||
|
||||
test("planInlineSync: severity then fp orders admissions deterministically", () => {
|
||||
const plan = planInlineSync({
|
||||
targets: [
|
||||
{ fp: "bbbb22", finding: finding({ file: "b.ts" }) },
|
||||
{ fp: "aaaa11", finding: finding() },
|
||||
{ fp: "cccc33", finding: finding({ severity: "critical", file: "c.ts" }) },
|
||||
],
|
||||
existing: [],
|
||||
maxComments: 2,
|
||||
teardown: true,
|
||||
});
|
||||
expect(plan.create.map((c) => c.fp)).toEqual(["cccc33", "aaaa11"]);
|
||||
});
|
||||
|
||||
// ---- body compare + marker scan ----
|
||||
|
||||
test("inlineBodiesEqual is CRLF-insensitive (the API returns CRLF bodies)", () => {
|
||||
expect(inlineBodiesEqual("a\r\nb", "a\nb")).toBe(true);
|
||||
expect(inlineBodiesEqual("a\nb", "a\nc")).toBe(false);
|
||||
});
|
||||
|
||||
test("hasUnquotedMarker: quoted copies do not count", () => {
|
||||
const marker = inlineCommentMarker(TAG, "deadbeef11");
|
||||
expect(hasUnquotedMarker(`${marker}\nbody`, marker)).toBe(true);
|
||||
expect(hasUnquotedMarker(`> ${marker}\nmy actual reply`, marker)).toBe(false);
|
||||
});
|
||||
|
||||
// ---- reply-matcher backstop ----
|
||||
|
||||
test("hasUnquotedInlineMarker: tag-independent, unquoted lines only", () => {
|
||||
expect(hasUnquotedInlineMarker(renderInlineCommentBody(finding(), "any-tag", "abcdef"))).toBe(
|
||||
true,
|
||||
);
|
||||
expect(hasUnquotedInlineMarker(`> <!-- any-tag:inline:fp=abcdef -->\nreal reply`)).toBe(false);
|
||||
});
|
||||
|
||||
test("matchReplies: our inline body is never matched as a reply, even with a maintainer login", () => {
|
||||
const f = finding();
|
||||
const fp = fingerprintFinding(f);
|
||||
const body = renderInlineCommentBody(f, "some-other-tag", fp); // carries an unquoted id: token
|
||||
const records = matchReplies(
|
||||
[{ id: 5, body, login: "bot", maintainer: true }],
|
||||
[{ finding: f, fp }],
|
||||
{
|
||||
match: "both",
|
||||
},
|
||||
);
|
||||
expect(records).toEqual([]);
|
||||
});
|
||||
|
||||
test("matchReplies: a quote-reply of an inline comment still matches", () => {
|
||||
const f = finding();
|
||||
const fp = fingerprintFinding(f);
|
||||
const quoted = renderInlineCommentBody(f, TAG, fp)
|
||||
.split("\n")
|
||||
.map((line) => `> ${line}`)
|
||||
.join("\n");
|
||||
const records = matchReplies(
|
||||
[{ id: 5, body: `${quoted}\n\nThis is intentional.`, login: "author", maintainer: false }],
|
||||
[{ finding: f, fp }],
|
||||
{ match: "both" },
|
||||
);
|
||||
expect(records.map((r) => r.fp)).toEqual([fp]);
|
||||
expect(records[0]!.citedId).toBe(false); // a quoted id never counts as citing
|
||||
});
|
||||
|
||||
// ---- threadFp ----
|
||||
|
||||
test("matchReplies: threadFp matches in every match mode and never sets citedId", () => {
|
||||
const f = finding();
|
||||
const fp = fingerprintFinding(f);
|
||||
for (const match of ["quote", "id", "both"] as const) {
|
||||
const records = matchReplies(
|
||||
[
|
||||
{
|
||||
id: -50,
|
||||
body: "we cache these on purpose",
|
||||
login: "author",
|
||||
maintainer: false,
|
||||
threadFp: fp,
|
||||
},
|
||||
],
|
||||
[{ finding: f, fp }],
|
||||
{ match },
|
||||
);
|
||||
expect(records.map((r) => r.fp)).toEqual([fp]);
|
||||
expect(records[0]!.citedId).toBe(false);
|
||||
expect(records[0]!.commentId).toBe(-50);
|
||||
}
|
||||
});
|
||||
|
||||
test("matchReplies: an unknown threadFp matches nothing", () => {
|
||||
const f = finding();
|
||||
const records = matchReplies(
|
||||
[{ id: -50, body: "x", login: "author", maintainer: false, threadFp: "ffffff" }],
|
||||
[{ finding: f, fp: fingerprintFinding(f) }],
|
||||
{ match: "both" },
|
||||
);
|
||||
expect(records).toEqual([]);
|
||||
});
|
||||
|
||||
// ---- cross-stream ids ----
|
||||
|
||||
test("replyIsNewer: magnitude within a stream, issue comments win across streams", () => {
|
||||
expect(replyIsNewer(200, 100)).toBe(true); // issue: newer wins
|
||||
expect(replyIsNewer(-200, -100)).toBe(true); // inline: raw 200 is newer than raw 100
|
||||
expect(replyIsNewer(-100, -200)).toBe(false);
|
||||
expect(replyIsNewer(100, -999)).toBe(true); // cross-stream: issue comment wins
|
||||
expect(replyIsNewer(-999, 100)).toBe(false);
|
||||
});
|
||||
|
||||
test("matchReplies: the newer inline reply wins within the inline stream", () => {
|
||||
const f = finding();
|
||||
const fp = fingerprintFinding(f);
|
||||
const mk = (id: number, login: string): ReplyComment => ({
|
||||
id,
|
||||
body: "",
|
||||
login,
|
||||
maintainer: false,
|
||||
threadFp: fp,
|
||||
});
|
||||
const records = matchReplies([mk(-100, "older"), mk(-200, "newer")], [{ finding: f, fp }], {
|
||||
match: "both",
|
||||
});
|
||||
expect(records[0]!.by).toBe("newer");
|
||||
});
|
||||
|
||||
test("applyPins: a cross-stream 'newer' maintainer reply never lifts a pin; same-stream magnitude does", () => {
|
||||
const record = (commentId: number): FeedbackRecord => ({
|
||||
fp: "aaaa11",
|
||||
by: "m",
|
||||
commentId,
|
||||
maintainer: true,
|
||||
citedId: true,
|
||||
applied: false,
|
||||
});
|
||||
// Pin taken against an inline reply (negative id): an issue reply cannot lift it.
|
||||
expect(applyPins([record(999)], [{ fp: "aaaa11", commentId: -100 }]).pins).toHaveLength(1);
|
||||
// A NEWER inline maintainer reply (raw 200 > raw 100) lifts it.
|
||||
expect(applyPins([record(-200)], [{ fp: "aaaa11", commentId: -100 }]).pins).toHaveLength(0);
|
||||
// An OLDER inline reply does not.
|
||||
expect(applyPins([record(-50)], [{ fp: "aaaa11", commentId: -100 }]).pins).toHaveLength(1);
|
||||
});
|
||||
|
||||
// ---- activeFindings ----
|
||||
|
||||
test("activeFindings: drops dismissed, reply-cleared, and requalified findings", () => {
|
||||
const keep = { finding: finding(), fp: "keep01" };
|
||||
const dismissed = { finding: finding({ file: "b.ts" }), fp: "gone01" };
|
||||
const cleared = { finding: finding({ file: "c.ts" }), fp: "gone02" };
|
||||
const requalified = {
|
||||
finding: finding({ file: "d.ts", requalifiedBy: { prNumber: 2, file: "x", reason: "r" } }),
|
||||
fp: "gone03",
|
||||
};
|
||||
const record: FeedbackRecord = {
|
||||
fp: "gone02",
|
||||
by: "author",
|
||||
commentId: 5,
|
||||
maintainer: false,
|
||||
applied: true,
|
||||
};
|
||||
const active = activeFindings(
|
||||
[keep, dismissed, cleared, requalified],
|
||||
[{ fp: "gone01" }],
|
||||
[record],
|
||||
);
|
||||
expect(active.map((entry) => entry.fp)).toEqual(["keep01"]);
|
||||
});
|
||||
|
||||
// ---- short form ----
|
||||
|
||||
test("renderMarkdown: an inlined finding renders short with the thread link; others render full", () => {
|
||||
const inlined = finding({ suggestion: "cache the observer" });
|
||||
const other = finding({ file: "b.ts", title: "Other finding", suggestion: "do the thing" });
|
||||
const review: CoordinatorOutput = {
|
||||
decision: "approve_with_comments",
|
||||
findings: [inlined, other],
|
||||
summary: "s",
|
||||
incomplete: [],
|
||||
};
|
||||
const link: LinkContext = {
|
||||
repo: "o/r",
|
||||
prNumber: 1,
|
||||
inlineUrls: new Map([
|
||||
[fingerprintFinding(inlined), "https://github.com/o/r/pull/1#discussion_r123"],
|
||||
]),
|
||||
};
|
||||
const body = renderMarkdown(review, TAG, [], link);
|
||||
expect(body).toContain("[inline comment](https://github.com/o/r/pull/1#discussion_r123)");
|
||||
// Short form: the one-line rationale survives, the suggestion moves inline-only.
|
||||
expect(body).toContain("getInstances() restarts the push token observer");
|
||||
expect(body).not.toContain("cache the observer");
|
||||
// The non-inlined finding keeps its full form.
|
||||
expect(body).toContain("**Suggestion:** do the thing");
|
||||
});
|
||||
|
||||
test("renderMarkdown: a non-github inline URL is ignored (full form)", () => {
|
||||
const f = finding({ suggestion: "s1" });
|
||||
const review: CoordinatorOutput = {
|
||||
decision: "approve_with_comments",
|
||||
findings: [f],
|
||||
summary: "s",
|
||||
incomplete: [],
|
||||
};
|
||||
const link: LinkContext = {
|
||||
repo: "o/r",
|
||||
prNumber: 1,
|
||||
inlineUrls: new Map([[fingerprintFinding(f), "https://evil.example/x"]]),
|
||||
};
|
||||
const body = renderMarkdown(review, TAG, [], link);
|
||||
expect(body).not.toContain("evil.example");
|
||||
expect(body).toContain("**Suggestion:** s1");
|
||||
});
|
||||
+16
-1
@@ -563,6 +563,7 @@ async function runLegacyCi(
|
||||
breakGlassMarker: config.breakGlassMarker,
|
||||
cwd,
|
||||
feedback: config.feedback,
|
||||
inline: config.inline,
|
||||
headSha,
|
||||
});
|
||||
|
||||
@@ -685,6 +686,10 @@ async function runLegacyCi(
|
||||
findings: [],
|
||||
summary: `⚠️ The AI reviewer failed to run, so this change was **not** reviewed:\n\n> ${publicFailureReason(error)}`,
|
||||
incomplete: [],
|
||||
// A failed run's empty findings list proves nothing: this flag is what keeps
|
||||
// the inline sync additive-only (no thread teardown) — and it renders the
|
||||
// accurate "No review" decision label.
|
||||
couldNotComplete: true,
|
||||
});
|
||||
} catch (postError) {
|
||||
process.stderr.write(
|
||||
@@ -701,6 +706,10 @@ function failureReview(scopeName: string, reason: string): CoordinatorOutput {
|
||||
findings: [],
|
||||
summary: `⚠️ The AI reviewer failed to run for scope **${scopeName}**, so those changes were **not** reviewed:\n\n> ${reason}`,
|
||||
incomplete: [],
|
||||
// Empty-but-not-clean: keeps the inline sync additive-only for the whole
|
||||
// aggregate (a failed scope's findings may still be live on the PR) and renders
|
||||
// the accurate "No review" label in the scope table.
|
||||
couldNotComplete: true,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -889,6 +898,7 @@ async function runRoutedCi(
|
||||
// Root-only feedback config (see loadScopeConfig): lets a reporter posting with
|
||||
// no explicit records match replies itself (annotate mode) at report time.
|
||||
feedback: rootConfig.feedback,
|
||||
inline: rootConfig.inline,
|
||||
headSha,
|
||||
});
|
||||
|
||||
@@ -1123,7 +1133,12 @@ async function runRoutedCi(
|
||||
aggState?.pins,
|
||||
)
|
||||
: undefined;
|
||||
await aggregate.reportAggregate(finalResults, resolution.unmatched, aggFeedback);
|
||||
// A --scopes partial run carries the other scopes out of (possibly truncated)
|
||||
// embedded state, so its target set is incomplete: inline teardown must not
|
||||
// run (failed scopes are handled inside reportAggregate itself).
|
||||
await aggregate.reportAggregate(finalResults, resolution.unmatched, aggFeedback, {
|
||||
inlineTeardown: !scopesFilter,
|
||||
});
|
||||
}
|
||||
// Clean up any per-scope comments from a previous per-scope run. A partial run
|
||||
// only ever touches the named scopes' comments.
|
||||
|
||||
@@ -98,6 +98,8 @@ export async function dismissCommand(argv: string[], mode: "add" | "remove"): Pr
|
||||
// feedback policy in force now — without it a reply-cleared finding would be
|
||||
// un-hidden by an unrelated /dismiss.
|
||||
feedback: config.feedback,
|
||||
// Lets the dismissal stub/revive the finding's inline thread (additive-only).
|
||||
inline: config.inline,
|
||||
});
|
||||
const result = await reporter.applyDismissal(
|
||||
mode === "add" ? args.ids : [],
|
||||
|
||||
@@ -147,6 +147,7 @@ export async function postReviewCommand(argv: string[]): Promise<void> {
|
||||
breakGlassMarker: config.breakGlassMarker,
|
||||
cwd,
|
||||
feedback: config.feedback,
|
||||
inline: config.inline,
|
||||
headSha,
|
||||
});
|
||||
const result = await publishDeferredReview(reporter, artifact, async () => {
|
||||
|
||||
@@ -276,6 +276,8 @@ export async function reviewCommand(argv: string[]): Promise<void> {
|
||||
cwd,
|
||||
// Root-only feedback config (loadScopeConfig inherits it from the root).
|
||||
feedback: config.feedback,
|
||||
// Root-only too (loadScopeConfig inherits it from the root).
|
||||
inline: config.inline,
|
||||
headSha: await reviewedHeadSha(source),
|
||||
})
|
||||
: null;
|
||||
@@ -361,6 +363,7 @@ export async function reviewCommand(argv: string[]): Promise<void> {
|
||||
breakGlassMarker: config.breakGlassMarker,
|
||||
cwd,
|
||||
feedback: config.feedback,
|
||||
inline: config.inline,
|
||||
headSha,
|
||||
})
|
||||
: null;
|
||||
|
||||
+11
-1
@@ -39,6 +39,12 @@ const FEEDBACK_CONFIG_DEFAULTS: LoadedConfig["feedback"] = {
|
||||
maxAdjudications: 10,
|
||||
};
|
||||
|
||||
/** Inline-comment config for a scope load (where `inline` is schema-rejected). */
|
||||
const INLINE_CONFIG_DEFAULTS: LoadedConfig["inline"] = {
|
||||
enabled: false,
|
||||
maxComments: 20,
|
||||
};
|
||||
|
||||
/** Research defaults for a scope load (where `research` is schema-rejected). */
|
||||
const RESEARCH_CONFIG_DEFAULTS: LoadedConfig["research"] = {
|
||||
enabled: false,
|
||||
@@ -76,7 +82,7 @@ export interface LoadConfigOptions {
|
||||
/** Parsed config with the centrally-locked keys optional (scope configs omit them). */
|
||||
type ParsedConfig = Omit<
|
||||
RawReviewConfig,
|
||||
"auth" | "breakGlass" | "commentTag" | "stack" | "feedback" | "research"
|
||||
"auth" | "breakGlass" | "commentTag" | "stack" | "feedback" | "research" | "inline"
|
||||
> & {
|
||||
auth?: RawReviewConfig["auth"];
|
||||
breakGlass?: RawReviewConfig["breakGlass"];
|
||||
@@ -84,6 +90,7 @@ type ParsedConfig = Omit<
|
||||
stack?: RawReviewConfig["stack"];
|
||||
feedback?: RawReviewConfig["feedback"];
|
||||
research?: RawReviewConfig["research"];
|
||||
inline?: RawReviewConfig["inline"];
|
||||
};
|
||||
|
||||
export function hasConfig(repoRoot: string, options: LoadConfigOptions = {}): boolean {
|
||||
@@ -219,6 +226,8 @@ async function loadConfigDir(
|
||||
// for a scope config and the defaults stand in (unused — the command layer
|
||||
// reads the ROOT config's feedback values; the comment lifecycle is global).
|
||||
feedback: parsed.feedback ?? FEEDBACK_CONFIG_DEFAULTS,
|
||||
// Root-only, same reasoning as feedback.
|
||||
inline: parsed.inline ?? INLINE_CONFIG_DEFAULTS,
|
||||
};
|
||||
return { config, raw: rawObject };
|
||||
}
|
||||
@@ -386,6 +395,7 @@ export async function loadScopeConfig(
|
||||
// run the default policy instead of the repo's real one.
|
||||
stack: rootConfig.stack,
|
||||
feedback: rootConfig.feedback,
|
||||
inline: rootConfig.inline,
|
||||
research: rootConfig.research,
|
||||
scopeName: scope.name,
|
||||
};
|
||||
|
||||
@@ -224,6 +224,20 @@ export const ReviewConfigSchema = z.object({
|
||||
protectedCategories: ["secrets", "security"],
|
||||
maxAdjudications: 10,
|
||||
}),
|
||||
// Inline PR review comments for findings anchored to a line in the diff: the
|
||||
// main comment stays the durable state store and renders such findings short,
|
||||
// linking to the inline thread. ROOT-ONLY (the comment lifecycle is global) and
|
||||
// off by default — a feature that mutates N comments per run earns trust with
|
||||
// field data first. Turning it off later leaves existing threads up (documented);
|
||||
// clear() still sweeps them on comment-mode switches.
|
||||
inline: z
|
||||
.object({
|
||||
enabled: z.boolean().default(false),
|
||||
// Cap on inline comments PER REPORTER: per PR in single/legacy comment mode,
|
||||
// per scope in per-scope mode. Bounds create-notification fan-out and API use.
|
||||
maxComments: z.number().int().positive().default(20),
|
||||
})
|
||||
.default({ enabled: false, maxComments: 20 }),
|
||||
});
|
||||
export type RawReviewConfig = z.infer<typeof ReviewConfigSchema>;
|
||||
|
||||
@@ -335,6 +349,7 @@ export const ScopeReviewConfigSchema = ReviewConfigSchema.omit({
|
||||
stack: true,
|
||||
feedback: true,
|
||||
research: true,
|
||||
inline: true,
|
||||
}).extend({
|
||||
auth: z
|
||||
.never({ error: "auth is locked to the root config; remove it from this scope config" })
|
||||
@@ -364,6 +379,12 @@ export const ScopeReviewConfigSchema = ReviewConfigSchema.omit({
|
||||
"research is locked to the root config because it starts a trusted host process; remove it from this scope config",
|
||||
})
|
||||
.optional(),
|
||||
inline: z
|
||||
.never({
|
||||
error:
|
||||
"inline is locked to the root config (the comment lifecycle is global); remove it from this scope config",
|
||||
})
|
||||
.optional(),
|
||||
});
|
||||
export type RawScopeReviewConfig = z.infer<typeof ScopeReviewConfigSchema>;
|
||||
|
||||
@@ -468,4 +489,9 @@ export interface LoadedConfig {
|
||||
protectedCategories: Category[];
|
||||
maxAdjudications: number;
|
||||
};
|
||||
/** Root-only inline-comment settings (see ReviewConfigSchema.inline). */
|
||||
inline: {
|
||||
enabled: boolean;
|
||||
maxComments: number;
|
||||
};
|
||||
}
|
||||
@@ -56,6 +56,10 @@ export function reviewPostingConfigFingerprint(config: LoadedConfig): string {
|
||||
commentTag: config.commentTag,
|
||||
breakGlassMarker: config.breakGlassMarker,
|
||||
feedback: config.feedback,
|
||||
// Inline comments change what posting does (N review-comment mutations), so a
|
||||
// saved review must not post under a different inline policy than it was
|
||||
// saved with.
|
||||
inline: config.inline,
|
||||
}),
|
||||
)
|
||||
.digest("hex");
|
||||
|
||||
@@ -35,6 +35,16 @@ export interface LinkContext {
|
||||
repo: string; // owner/repo
|
||||
prNumber: number;
|
||||
diffLines?: Map<string, Set<number>>;
|
||||
/**
|
||||
* Permalinks to this run's live inline finding comments, keyed by the SAME id the
|
||||
* comment renders (plain fingerprint, or scope-namespaced for an aggregate). A
|
||||
* finding with an entry renders in SHORT form — title, links, one-line rationale —
|
||||
* because its full text lives in the inline thread; a finding without one renders
|
||||
* in full exactly as before. Callers must pass a per-render COPY of a shared
|
||||
* LinkContext (`{ ...link, inlineUrls }`), never mutate the shared object: the
|
||||
* routed ci flow shares one context across every scope reporter.
|
||||
*/
|
||||
inlineUrls?: Map<string, string>;
|
||||
/**
|
||||
* The PR base commit SHA (the branch the PR targets, e.g. `main`). When a finding
|
||||
* points at code NOT in the diff — a caller or helper the PR merely references,
|
||||
@@ -121,6 +131,29 @@ export function commentMarker(tag: string): string {
|
||||
return `<!-- ${tag} -->`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Per-finding identity marker of an inline (PR review) comment, always rendered at
|
||||
* byte 0 of the body. Substring-safe against `commentMarker(tag)` and any derived
|
||||
* scope tag (`<tag>:<scope>`) in both directions — the closing ` -->` differs —
|
||||
* including a scope literally named `inline`.
|
||||
*/
|
||||
export function inlineCommentMarker(tag: string, fp: string): string {
|
||||
return `<!-- ${tag}:inline:fp=${fp} -->`;
|
||||
}
|
||||
|
||||
/**
|
||||
* The fingerprint an inline comment of OURS carries, or null. Anchored at byte 0
|
||||
* and constrained to the fingerprint alphabet, so a marker echoed later in a body
|
||||
* (untrusted prose, a quote-reply) can never parse as identity — mechanical
|
||||
* safety, not just the incidental "our renderer emits it first". Identity itself
|
||||
* is author + this marker (see the reporter); the parse alone proves nothing.
|
||||
*/
|
||||
export function parseInlineMarkerFp(body: string, tag: string): string | null {
|
||||
const escapedTag = tag.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
|
||||
const match = body.match(new RegExp(`^<!-- ${escapedTag}:inline:fp=([a-f0-9]{6,64}) -->`));
|
||||
return match ? match[1]! : null;
|
||||
}
|
||||
|
||||
// @ref LLP 0011#forged-state-markers [implements] — the first-match parsers make an earlier forged marker win, so untrusted prose never keeps a raw `<!--`
|
||||
/**
|
||||
* Neutralize anything that could impersonate this reviewer's embedded state
|
||||
@@ -348,12 +381,49 @@ function sourceLabel(value: string): string {
|
||||
.replace(/([\\[\]])/g, "\\$1");
|
||||
}
|
||||
|
||||
/** Longest one-line rationale excerpt the short form keeps in the main comment. */
|
||||
const SHORT_RATIONALE_CHARS = 160;
|
||||
|
||||
/**
|
||||
* A rationale reduced to one plain line for the short (inlined) form: whitespace
|
||||
* collapsed, every `<` escaped so a truncated HTML block (`<details>`) can never
|
||||
* leave an unclosed tag in the comment, cut at a word boundary with an ellipsis.
|
||||
* This excerpt is the audit trail that survives the PR author collapsing or
|
||||
* resolving the inline thread, so it must always be non-empty when the rationale is.
|
||||
*/
|
||||
export function oneLineRationale(text: string): string {
|
||||
const flat = text.replace(/</g, "<").replace(/\s+/g, " ").trim();
|
||||
if (flat.length <= SHORT_RATIONALE_CHARS) {
|
||||
return flat;
|
||||
}
|
||||
const cut = flat.slice(0, SHORT_RATIONALE_CHARS);
|
||||
const atWord = cut.slice(0, cut.lastIndexOf(" "));
|
||||
return `${atWord.length > 0 ? atWord : cut}…`;
|
||||
}
|
||||
|
||||
/** Only ever link to github.com from a stored/returned URL (see REPLY_URL_RE). */
|
||||
function validInlineUrl(link: LinkContext | undefined, id: string): string | null {
|
||||
const url = link?.inlineUrls?.get(id);
|
||||
return url && REPLY_URL_RE.test(url) ? url : null;
|
||||
}
|
||||
|
||||
function renderFindingLines(
|
||||
finding: Finding,
|
||||
link?: LinkContext,
|
||||
id: string = fingerprintFinding(finding),
|
||||
reply?: FeedbackRecord,
|
||||
): string[] {
|
||||
const inlineUrl = validInlineUrl(link, id);
|
||||
if (inlineUrl) {
|
||||
// Short form: the full text lives in the inline thread on the flagged line.
|
||||
// The one-line rationale stays here as the durable audit trail (the author
|
||||
// can collapse/resolve the thread; they cannot edit this comment).
|
||||
return [
|
||||
`- **${stripStateMarkers(finding.title)}** — ${location(finding, link)} _(${finding.category})_ · \`id:${id}\` · 💬 [inline comment](${inlineUrl})${replyAnnotation(reply)}`,
|
||||
...indentContinuation(oneLineRationale(stripStateMarkers(finding.rationale))),
|
||||
"",
|
||||
];
|
||||
}
|
||||
const out = [
|
||||
`- **${stripStateMarkers(finding.title)}** — ${location(finding, link)} _(${finding.category})_ · \`id:${id}\`${replyAnnotation(reply)}`,
|
||||
...indentContinuation(stripStateMarkers(finding.rationale)),
|
||||
@@ -376,6 +446,53 @@ function renderFindingLines(
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* Full body of one inline (PR review) finding comment: identity marker at byte 0,
|
||||
* then the finding rendered whole — everything model-written passes
|
||||
* stripStateMarkers, same as the main comment. The footer teaches the clearing
|
||||
* flow (an UNQUOTED id citation), since the main comment only shows the short form.
|
||||
* The body must never contain `:state=`/`:fingerprints=` (it would be dropped as
|
||||
* our own main comment by the reply matcher's backstop) — it doesn't.
|
||||
*/
|
||||
export function renderInlineCommentBody(finding: Finding, tag: string, fp: string): string {
|
||||
const lines = [
|
||||
inlineCommentMarker(tag, fp),
|
||||
`**${severityHeading(finding.severity)}: ${stripStateMarkers(finding.title)}** _(${finding.category})_ · \`id:${fp}\``,
|
||||
"",
|
||||
stripStateMarkers(finding.rationale),
|
||||
];
|
||||
if (finding.sources?.length) {
|
||||
const sources = finding.sources
|
||||
.map((source) => `[${sourceLabel(source.title)}](<${source.url}>)`)
|
||||
.join(", ");
|
||||
lines.push("", `**Sources:** ${sources}`);
|
||||
}
|
||||
if (finding.suggestion) {
|
||||
lines.push("", `**Suggestion:** ${stripStateMarkers(finding.suggestion)}`);
|
||||
}
|
||||
lines.push(
|
||||
"",
|
||||
"---",
|
||||
`_🤖 AI review finding — reply here to respond; write \`id:${fp}\` (outside any quote) in your reply to formally answer it. Full review in the main PR comment._`,
|
||||
);
|
||||
return lines.join("\n");
|
||||
}
|
||||
|
||||
/**
|
||||
* What a no-longer-tracked inline comment is patched to when its thread has human
|
||||
* replies (a bare one is deleted instead). Deliberately NEUTRAL: a finding leaves
|
||||
* the inline set for many reasons that are not resolution — capped out, its line
|
||||
* left the diff, aggregate-state truncation — so this text must never assert
|
||||
* "resolved" or "dismissed". The marker stays, so a returning finding revives the
|
||||
* same thread instead of opening a new one.
|
||||
*/
|
||||
export function inlineStubBody(tag: string, fp: string): string {
|
||||
return [
|
||||
inlineCommentMarker(tag, fp),
|
||||
"_🤖 This finding is no longer tracked inline — see the main review comment for current status._",
|
||||
].join("\n");
|
||||
}
|
||||
|
||||
// @ref LLP 0012#run-points-command-and-review [implements] — setup advice renders outside the findings list, so it never blocks
|
||||
/** Advice about the reviewer's own setup (stale refs, cited code this PR moves). */
|
||||
function setupNote(notes: string[] = []): string[] {
|
||||
|
||||
+50
-2
@@ -3,6 +3,9 @@ import type { FeedbackRecord, Finding } from "./schema.js";
|
||||
|
||||
/** One human comment on the PR, as the reporter hands it over. */
|
||||
export interface ReplyComment {
|
||||
/** Issue-comment id, or the NEGATED PR-review-comment id for an inline reply.
|
||||
* The two GitHub id sequences are independent, so negation keeps them
|
||||
* collision-free in every stored/compared FeedbackRecord.commentId. */
|
||||
id: number;
|
||||
body: string;
|
||||
login: string;
|
||||
@@ -12,6 +15,15 @@ export interface ReplyComment {
|
||||
* Absent/false is fail-closed — an unresolved author can never clear a finding. */
|
||||
author?: boolean;
|
||||
url?: string;
|
||||
/**
|
||||
* For an inline (PR review) reply: the fingerprint of OUR finding whose thread it
|
||||
* answers, resolved by the reporter from `in_reply_to_id` chaining to an
|
||||
* author-verified, marker-anchored inline comment of ours. A structural signal
|
||||
* chosen by the replier (they replied to that thread), so it matches in EVERY
|
||||
* `match` mode — but it never sets `citedId`: clearing still requires the
|
||||
* replier's own unquoted `id:` token.
|
||||
*/
|
||||
threadFp?: string;
|
||||
}
|
||||
|
||||
/** Blockquote lines read per comment. A reply that quotes half the review is
|
||||
@@ -35,6 +47,23 @@ const FINDING_ID_RE = /\bid:([a-f0-9]{6,64})\b/gi;
|
||||
*/
|
||||
const OWN_COMMENT_RE = /<!--[^\n]*:(?:state|fingerprints)=/;
|
||||
|
||||
/** The tag-independent shape of an inline finding comment's identity marker. */
|
||||
const INLINE_MARKER_RE = /<!--[^\n]*:inline:fp=/;
|
||||
|
||||
/**
|
||||
* Tag-independent backstop for OUR inline finding comments, which carry an
|
||||
* UNQUOTED `id:<fp>` token in the body (so a maintainer can copy it into a
|
||||
* reply). If the author/tag filters ever miss one — a bot-login fallback, a
|
||||
* commentTag rename, a second posting identity — matching it as a reply would
|
||||
* let the review cite (and, with maintainer association, CLEAR) its own
|
||||
* finding. Only UNQUOTED lines count: GitHub's "Quote reply" copies our marker
|
||||
* verbatim behind `> `, and that comment is a genuine human reply that must
|
||||
* still be matched.
|
||||
*/
|
||||
export function hasUnquotedInlineMarker(body: string): boolean {
|
||||
return body.split("\n").some((line) => !/^\s*>/.test(line) && INLINE_MARKER_RE.test(line));
|
||||
}
|
||||
|
||||
/**
|
||||
* Markdown → comparable text: link text without the target, no backticks or
|
||||
* emphasis marks, collapsed whitespace, lowercase, no trailing punctuation. A
|
||||
@@ -82,6 +111,20 @@ export function extractQuotedLines(body: string): string[] {
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* Is `candidate` a more recent reply than `incumbent`? Issue comments carry
|
||||
* positive ids; inline (PR review) replies carry NEGATED ids, so within the
|
||||
* inline stream recency is the raw id's magnitude, not the signed value. The two
|
||||
* sequences are independent, so cross-stream recency is undecidable — the issue
|
||||
* comment wins deterministically (the main thread is the formal reply channel).
|
||||
*/
|
||||
export function replyIsNewer(candidate: number, incumbent: number): boolean {
|
||||
if (candidate >= 0 === incumbent >= 0) {
|
||||
return Math.abs(candidate) > Math.abs(incumbent);
|
||||
}
|
||||
return candidate >= 0;
|
||||
}
|
||||
|
||||
/** The `id:<hex>` tokens a comment cites, lowercased and deduped. */
|
||||
export function extractFindingIds(body: string): string[] {
|
||||
const ids = [...body.matchAll(FINDING_ID_RE)].map((match) => match[1]!.toLowerCase());
|
||||
@@ -137,7 +180,7 @@ export function matchReplies(
|
||||
|
||||
const newest = new Map<string, FeedbackRecord>();
|
||||
for (const comment of comments) {
|
||||
if (OWN_COMMENT_RE.test(comment.body)) {
|
||||
if (OWN_COMMENT_RE.test(comment.body) || hasUnquotedInlineMarker(comment.body)) {
|
||||
continue;
|
||||
}
|
||||
const matched = new Set<string>();
|
||||
@@ -145,6 +188,11 @@ export function matchReplies(
|
||||
// quote-matched reply still records WHETHER it cites the finding, because that is
|
||||
// what decides clearing.
|
||||
const cited = new Set(extractCitedFindingIds(comment.body));
|
||||
// An inline reply names its finding structurally — the replier chose which
|
||||
// thread to answer — so it matches in every `match` mode. Never sets `citedId`.
|
||||
if (comment.threadFp && known.has(comment.threadFp)) {
|
||||
matched.add(comment.threadFp);
|
||||
}
|
||||
if (opts.match !== "quote") {
|
||||
for (const id of extractFindingIds(comment.body)) {
|
||||
if (known.has(id)) {
|
||||
@@ -168,7 +216,7 @@ export function matchReplies(
|
||||
}
|
||||
for (const fp of matched) {
|
||||
const previous = newest.get(fp);
|
||||
if (previous && previous.commentId >= comment.id) {
|
||||
if (previous && !replyIsNewer(comment.id, previous.commentId)) {
|
||||
continue;
|
||||
}
|
||||
newest.set(fp, {
|
||||
|
||||
+10
-1
@@ -428,7 +428,16 @@ export function applyPins(
|
||||
pin.commentId === undefined ||
|
||||
!records.some(
|
||||
(record) =>
|
||||
record.fp === pin.fp && record.maintainer === true && record.commentId > pin.commentId!,
|
||||
record.fp === pin.fp &&
|
||||
record.maintainer === true &&
|
||||
// "Posted after the pin" is only decidable within one id sequence. Issue
|
||||
// comments carry positive ids and inline (PR review) replies carry NEGATED
|
||||
// ones (see the reporter's replyComments); the two sequences are
|
||||
// independent, so a cross-stream comparison says nothing about time — keep
|
||||
// the pin. Within a stream, recency is the raw id's magnitude (negation
|
||||
// inverts the signed order for inline ids).
|
||||
record.commentId >= 0 === pin.commentId! >= 0 &&
|
||||
Math.abs(record.commentId) > Math.abs(pin.commentId!),
|
||||
),
|
||||
);
|
||||
const pinnedFps = new Set(kept.map((pin) => pin.fp));
|
||||
|
||||
+724
-5
@@ -4,12 +4,16 @@ import { tmpdir } from "node:os";
|
||||
import path from "node:path";
|
||||
|
||||
import { resolveTrustedTool, run } from "../core/exec.js";
|
||||
import { errorMessage } from "../core/util.js";
|
||||
import { parseUnifiedDiff } from "../core/diff.js";
|
||||
import {
|
||||
buildDiffLineIndex,
|
||||
commentMarker,
|
||||
inlineStubBody,
|
||||
parseInlineMarkerFp,
|
||||
parseReviewState,
|
||||
renderAggregateMarkdown,
|
||||
renderInlineCommentBody,
|
||||
renderMarkdown,
|
||||
} from "../core/render.js";
|
||||
import type { LinkContext, ReviewState, ScopeReviewResult } from "../core/render.js";
|
||||
@@ -17,7 +21,13 @@ import { matchReplies } from "../core/responses.js";
|
||||
import type { ReplyComment } from "../core/responses.js";
|
||||
import { dropStaleVerdict, feedbackApplied, feedbackNeedsPrAuthor } from "../core/adjudicate.js";
|
||||
import type { AdjudicationItem } from "../core/adjudicate.js";
|
||||
import { applyPins, collectPins, fingerprintFinding, scopedFingerprint } from "../core/schema.js";
|
||||
import {
|
||||
applyPins,
|
||||
collectPins,
|
||||
fingerprintFinding,
|
||||
scopedFingerprint,
|
||||
SEVERITY_RANK,
|
||||
} from "../core/schema.js";
|
||||
import type {
|
||||
CoordinatorOutput,
|
||||
DismissalRecord,
|
||||
@@ -52,6 +62,13 @@ export interface GitHubReporterOptions {
|
||||
* the reporter behaves exactly as before until a caller opts in.
|
||||
*/
|
||||
feedback?: LoadedConfig["feedback"];
|
||||
/**
|
||||
* Root-only inline-comment config (see LoadedConfig.inline). Absent ⇒ the
|
||||
* reporter never touches PR review comments at all (zero extra API calls);
|
||||
* present with `enabled: false` ⇒ no sync, but `clear()` still sweeps this
|
||||
* tag's inline comments so a comment-mode switch or a disable cleans up.
|
||||
*/
|
||||
inline?: LoadedConfig["inline"];
|
||||
/**
|
||||
* The head commit this run reviews (the PR's immutable head OID, from the source's
|
||||
* metadata). Feedback merging binds a stored verdict to the source it judged, so a
|
||||
@@ -148,6 +165,188 @@ export interface IssueComment {
|
||||
html_url?: string;
|
||||
}
|
||||
|
||||
/** One PR review (inline/diff) comment, as the pulls comments API returns it. */
|
||||
export interface ReviewCommentApi {
|
||||
id: number;
|
||||
body?: string;
|
||||
path?: string;
|
||||
line?: number | null;
|
||||
user?: { login?: string };
|
||||
html_url?: string;
|
||||
/** Present on replies; points at the thread's root comment. */
|
||||
in_reply_to_id?: number;
|
||||
author_association?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Burst-collapsing cache over the paginated review-comments listing, shared across
|
||||
* reporter INSTANCES per PR (same motivation and key shape as prAuthorByPr): routed
|
||||
* single-mode CI builds a fresh reporter per scope for its clear() reconciliation,
|
||||
* and per-instance caching would re-fetch the identical list once per scope. The
|
||||
* short TTL and mutation invalidation mirror fetchAllComments.
|
||||
*/
|
||||
const reviewCommentsByPr = new Map<string, { at: number; comments: Promise<ReviewCommentApi[]> }>();
|
||||
|
||||
/** An existing inline comment of OURS (author + anchored marker), as the planner sees it. */
|
||||
export interface ExistingInlineComment {
|
||||
id: number;
|
||||
fp: string;
|
||||
body: string;
|
||||
url?: string;
|
||||
/** Any listed comment replies to it (`in_reply_to_id` === its id). */
|
||||
hasReplies: boolean;
|
||||
}
|
||||
|
||||
/** What one inline sync run will do. Bodies are rendered by the executor. */
|
||||
export interface InlineSyncPlan {
|
||||
/** Findings to POST a new inline comment for (needs a commit id). */
|
||||
create: Array<{ fp: string; finding: Finding }>;
|
||||
/** Live threads whose body converges to the finding's current render. */
|
||||
patch: Array<{ comment: ExistingInlineComment; finding: Finding }>;
|
||||
/** No-longer-tracked threads WITH replies: patch to the neutral stub. */
|
||||
stub: ExistingInlineComment[];
|
||||
/** No-longer-tracked threads with no replies (and older duplicates): delete. */
|
||||
remove: ExistingInlineComment[];
|
||||
}
|
||||
|
||||
// @ref LLP 0011#suppression-is-never-silent [constrained-by] — leaving the inline set is
|
||||
// not resolution: teardown only stubs/deletes when the caller proved the target set is
|
||||
// complete, and the stub text (see inlineStubBody) never claims resolved/dismissed
|
||||
/**
|
||||
* Decide the inline mutations for one sync, purely. `targets` are the ACTIVE,
|
||||
* diff-anchored findings; `existing` are OUR live inline comments.
|
||||
*
|
||||
* The cap is STICKY and deterministic: candidates order by severity rank then fp,
|
||||
* and fps that already have a live thread fill slots before new ones are admitted —
|
||||
* a live thread is never evicted (and its replies destroyed) just to create a
|
||||
* different one, and a tie at the boundary cannot flip membership run to run.
|
||||
*
|
||||
* `teardown: false` (a failed/partial/carried run whose target set may be
|
||||
* incomplete) makes the plan additive-only: no stubs, no deletes of live threads —
|
||||
* except older DUPLICATES of a kept fp (a crash window between create and list),
|
||||
* which are ours and redundant whatever the target set says.
|
||||
*/
|
||||
export function planInlineSync(opts: {
|
||||
targets: Array<{ fp: string; finding: Finding }>;
|
||||
existing: ExistingInlineComment[];
|
||||
maxComments: number;
|
||||
teardown: boolean;
|
||||
}): InlineSyncPlan {
|
||||
const ordered = [...opts.targets].sort(
|
||||
(a, b) =>
|
||||
SEVERITY_RANK[a.finding.severity] - SEVERITY_RANK[b.finding.severity] ||
|
||||
a.fp.localeCompare(b.fp),
|
||||
);
|
||||
|
||||
// Newest comment per fp is the live thread; older ones are duplicates.
|
||||
const liveByFp = new Map<string, ExistingInlineComment>();
|
||||
const duplicates: ExistingInlineComment[] = [];
|
||||
for (const comment of [...opts.existing].sort((a, b) => a.id - b.id)) {
|
||||
const previous = liveByFp.get(comment.fp);
|
||||
if (previous) {
|
||||
duplicates.push(previous);
|
||||
}
|
||||
liveByFp.set(comment.fp, comment);
|
||||
}
|
||||
|
||||
const selected: Array<{ fp: string; finding: Finding }> = [];
|
||||
for (const entry of ordered) {
|
||||
if (selected.length >= opts.maxComments) {
|
||||
break;
|
||||
}
|
||||
if (liveByFp.has(entry.fp)) {
|
||||
selected.push(entry);
|
||||
}
|
||||
}
|
||||
for (const entry of ordered) {
|
||||
if (selected.length >= opts.maxComments) {
|
||||
break;
|
||||
}
|
||||
if (!liveByFp.has(entry.fp)) {
|
||||
selected.push(entry);
|
||||
}
|
||||
}
|
||||
|
||||
const plan: InlineSyncPlan = { create: [], patch: [], stub: [], remove: [] };
|
||||
const selectedFps = new Set(selected.map((entry) => entry.fp));
|
||||
for (const entry of selected) {
|
||||
const live = liveByFp.get(entry.fp);
|
||||
if (live) {
|
||||
plan.patch.push({ comment: live, finding: entry.finding });
|
||||
} else {
|
||||
plan.create.push(entry);
|
||||
}
|
||||
}
|
||||
for (const [fp, live] of liveByFp) {
|
||||
if (selectedFps.has(fp)) {
|
||||
continue;
|
||||
}
|
||||
if (!opts.teardown) {
|
||||
continue;
|
||||
}
|
||||
if (live.hasReplies) {
|
||||
plan.stub.push(live);
|
||||
} else {
|
||||
plan.remove.push(live);
|
||||
}
|
||||
}
|
||||
// A duplicate with human replies is still a conversation: stub it like any other
|
||||
// retired thread rather than deleting what people wrote in.
|
||||
for (const duplicate of duplicates) {
|
||||
if (duplicate.hasReplies) {
|
||||
plan.stub.push(duplicate);
|
||||
} else {
|
||||
plan.remove.push(duplicate);
|
||||
}
|
||||
}
|
||||
return plan;
|
||||
}
|
||||
|
||||
/** `\r\n`-insensitive body comparison: the API returns CRLF-normalized bodies. */
|
||||
export function inlineBodiesEqual(a: string, b: string): boolean {
|
||||
return a.replace(/\r\n/g, "\n") === b.replace(/\r\n/g, "\n");
|
||||
}
|
||||
|
||||
/** Does a body's UNQUOTED text carry this marker? Quoted (`> `) lines are the
|
||||
* replier quoting US, which is a genuine reply, not our comment. */
|
||||
export function hasUnquotedMarker(body: string, marker: string): boolean {
|
||||
return body.split("\n").some((line) => !/^\s*>/.test(line) && line.includes(marker));
|
||||
}
|
||||
|
||||
/**
|
||||
* The findings the main comment renders in its ACTIVE list — not dismissed, not
|
||||
* cleared by an applied reply, not requalified — which is exactly the inline sync's
|
||||
* target set. Mirrors the isDropped/kept split in renderMarkdown and
|
||||
* renderAggregateMarkdown (first record per fp wins, like matchedFeedback there).
|
||||
* Exported for tests.
|
||||
*/
|
||||
export function activeFindings(
|
||||
withFp: Array<{ finding: Finding; fp: string }>,
|
||||
dismissed: DismissalRecord[],
|
||||
records: FeedbackRecord[],
|
||||
): Array<{ finding: Finding; fp: string }> {
|
||||
const dismissedFps = new Set(dismissed.map((record) => record.fp));
|
||||
const appliedFps = new Set<string>();
|
||||
const seen = new Set<string>();
|
||||
for (const record of records) {
|
||||
if (!seen.has(record.fp)) {
|
||||
seen.add(record.fp);
|
||||
if (record.applied) {
|
||||
appliedFps.add(record.fp);
|
||||
}
|
||||
}
|
||||
}
|
||||
return withFp.filter(
|
||||
({ finding, fp }) => !dismissedFps.has(fp) && !appliedFps.has(fp) && !finding.requalifiedBy,
|
||||
);
|
||||
}
|
||||
|
||||
/** A per-render COPY of the link context carrying this run's inline thread URLs —
|
||||
* the routed ci flow shares one LinkContext across reporters, so never mutate it. */
|
||||
function withInlineUrls(link: LinkContext, inlineUrls: Map<string, string>): LinkContext {
|
||||
return inlineUrls.size > 0 ? { ...link, inlineUrls } : link;
|
||||
}
|
||||
|
||||
// @ref LLP 0011#suppression-is-never-silent [implements] — a verdict carries only while BOTH the reply and the reviewed source are unchanged
|
||||
/**
|
||||
* Carry a decision already recorded in the prior comment state onto the freshly
|
||||
@@ -490,16 +689,38 @@ export class GitHubReporter implements Reporter {
|
||||
? applyPins(feedback, pinsIn)
|
||||
: await this.computeFeedback(withFp, priorRecords, pinsIn);
|
||||
const link = await this.linkContextAsync();
|
||||
// Inline threads converge to the ACTIVE findings before the body renders, so
|
||||
// the short form can link each one. A failed run (couldNotComplete) posts an
|
||||
// empty findings list that proves nothing — additive-only, no teardown.
|
||||
const inlineUrls = await this.syncInline(activeFindings(withFp, dismissed, records), link, {
|
||||
teardown: review.couldNotComplete !== true,
|
||||
});
|
||||
await this.upsertComment(
|
||||
renderMarkdown(review, this.options.commentTag, dismissed, link, records, pins, inputHash),
|
||||
renderMarkdown(
|
||||
review,
|
||||
this.options.commentTag,
|
||||
dismissed,
|
||||
withInlineUrls(link, inlineUrls),
|
||||
records,
|
||||
pins,
|
||||
inputHash,
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
/** Post/update the aggregate multi-scope comment (comment:'single' mode). */
|
||||
/**
|
||||
* Post/update the aggregate multi-scope comment (comment:'single' mode).
|
||||
* `opts.inlineTeardown: false` marks a run whose result set is NOT the complete
|
||||
* truth about the PR — a `--scopes` partial run carries the other scopes out of
|
||||
* (possibly truncated) embedded state — so the inline sync must not stub/delete
|
||||
* threads it merely cannot see. Any failed scope forces the same, since its
|
||||
* findings list is empty without being clean.
|
||||
*/
|
||||
async reportAggregate(
|
||||
results: ScopeReviewResult[],
|
||||
unmatchedFiles: string[],
|
||||
feedback?: FeedbackRecord[],
|
||||
opts?: { inlineTeardown?: boolean },
|
||||
): Promise<void> {
|
||||
const existing = await this.findExistingComment();
|
||||
const state = existing ? parseReviewState(existing.body, this.options.commentTag) : null;
|
||||
@@ -518,12 +739,18 @@ export class GitHubReporter implements Reporter {
|
||||
? applyPins(feedback, pinsIn)
|
||||
: await this.computeFeedback(withFp, priorRecords, pinsIn);
|
||||
const link = await this.linkContextAsync();
|
||||
const teardown =
|
||||
(opts?.inlineTeardown ?? true) &&
|
||||
results.every((result) => result.review.couldNotComplete !== true);
|
||||
const inlineUrls = await this.syncInline(activeFindings(withFp, dismissed, records), link, {
|
||||
teardown,
|
||||
});
|
||||
await this.upsertComment(
|
||||
renderAggregateMarkdown(
|
||||
results,
|
||||
this.options.commentTag,
|
||||
dismissed,
|
||||
link,
|
||||
withInlineUrls(link, inlineUrls),
|
||||
{ unmatchedFiles },
|
||||
records,
|
||||
pins,
|
||||
@@ -698,10 +925,17 @@ export class GitHubReporter implements Reporter {
|
||||
* failed lookup gives, and under any other `dismiss` value nothing reads the flag.
|
||||
*/
|
||||
private async replyComments(): Promise<ReplyComment[]> {
|
||||
const [comments, ownLogin, prAuthor] = await Promise.all([
|
||||
// Inline replies are read only when the reporter carries an ENABLED inline
|
||||
// config: the `ecr feedback` crawl and every non-inline repo keep exactly the
|
||||
// API footprint they had.
|
||||
const inlineEnabled = this.options.inline?.enabled === true;
|
||||
const [comments, ownLogin, prAuthor, reviewComments] = await Promise.all([
|
||||
this.fetchAllComments(),
|
||||
this.resolveOwnLogin(),
|
||||
feedbackNeedsPrAuthor(this.options.feedback) ? this.resolvePrAuthor() : null,
|
||||
inlineEnabled
|
||||
? this.fetchAllReviewComments().catch(() => [] as ReviewCommentApi[])
|
||||
: ([] as ReviewCommentApi[]),
|
||||
]);
|
||||
const out: ReplyComment[] = [];
|
||||
for (const comment of comments) {
|
||||
@@ -723,6 +957,64 @@ export class GitHubReporter implements Reporter {
|
||||
...(comment.html_url ? { url: comment.html_url } : {}),
|
||||
});
|
||||
}
|
||||
if (inlineEnabled && reviewComments.length > 0) {
|
||||
// fp per OUR top-level inline comment (author + anchored marker), so a reply
|
||||
// threading to it names its finding structurally.
|
||||
const fpByRootId = new Map<number, string>();
|
||||
if (ownLogin) {
|
||||
for (const comment of reviewComments) {
|
||||
if (comment.in_reply_to_id != null || comment.user?.login !== ownLogin) {
|
||||
continue;
|
||||
}
|
||||
const fp = parseInlineMarkerFp(comment.body ?? "", this.options.commentTag);
|
||||
if (fp) {
|
||||
fpByRootId.set(comment.id, fp);
|
||||
}
|
||||
}
|
||||
}
|
||||
const byId = new Map(reviewComments.map((comment) => [comment.id, comment]));
|
||||
// `in_reply_to_id` points at the thread root, but chase a short chain anyway
|
||||
// so an unexpected reply-to-reply shape still resolves.
|
||||
const threadFpOf = (comment: ReviewCommentApi): string | undefined => {
|
||||
let parentId = comment.in_reply_to_id;
|
||||
for (let hop = 0; parentId != null && hop < 5; hop++) {
|
||||
const fp = fpByRootId.get(parentId);
|
||||
if (fp) {
|
||||
return fp;
|
||||
}
|
||||
parentId = byId.get(parentId)?.in_reply_to_id;
|
||||
}
|
||||
return undefined;
|
||||
};
|
||||
for (const comment of reviewComments) {
|
||||
const login = comment.user?.login;
|
||||
if (!login || (ownLogin && login === ownLogin)) {
|
||||
continue;
|
||||
}
|
||||
const body = comment.body ?? "";
|
||||
// Unquoted-line marker tests only: a GitHub "Quote reply" copies our marker
|
||||
// behind `> ` and is a genuine human reply that must still be matched.
|
||||
if (
|
||||
hasUnquotedMarker(body, this.marker) ||
|
||||
body.split("\n").some((line) => !/^\s*>/.test(line) && line.includes(":inline:fp="))
|
||||
) {
|
||||
continue;
|
||||
}
|
||||
const threadFp = threadFpOf(comment);
|
||||
out.push({
|
||||
// NEGATED: PR-review-comment ids are an independent sequence from issue
|
||||
// comments; negation keeps stored commentIds collision-free across streams
|
||||
// (and applyPins refuses cross-stream pin-lift comparisons).
|
||||
id: -comment.id,
|
||||
body,
|
||||
login,
|
||||
maintainer: MAINTAINER_ASSOCIATIONS.has(comment.author_association ?? ""),
|
||||
author: prAuthor != null && login === prAuthor,
|
||||
...(comment.html_url ? { url: comment.html_url } : {}),
|
||||
...(threadFp ? { threadFp } : {}),
|
||||
});
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
@@ -748,6 +1040,35 @@ export class GitHubReporter implements Reporter {
|
||||
for (const comment of await this.ownComments()) {
|
||||
await this.deleteComment(comment.id);
|
||||
}
|
||||
// Sweep this tag's inline threads too, gated on the config KEY being present
|
||||
// (not on `enabled`), so a comment-mode switch or a later disable still cleans
|
||||
// up — while callers that never wire `inline` (break-glass reporter, terminal
|
||||
// failure notes) keep their zero-review-API footprint. A thread humans replied
|
||||
// in is stubbed, never deleted. Fail-soft per thread.
|
||||
if (this.options.inline !== undefined) {
|
||||
let own: ExistingInlineComment[] = [];
|
||||
try {
|
||||
own = await this.ownInlineComments();
|
||||
} catch {
|
||||
return; // fail-soft: the main-comment clear above already happened
|
||||
}
|
||||
for (const comment of own) {
|
||||
try {
|
||||
if (comment.hasReplies) {
|
||||
const stub = inlineStubBody(this.options.commentTag, comment.fp);
|
||||
if (!inlineBodiesEqual(comment.body, stub)) {
|
||||
await this.patchReviewComment(comment.id, stub);
|
||||
}
|
||||
} else {
|
||||
await this.deleteReviewComment(comment.id);
|
||||
}
|
||||
} catch (error) {
|
||||
process.stderr.write(
|
||||
`inline sync: clear failed for one inline comment (skipped): ${errorMessage(error)}\n`,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -793,6 +1114,365 @@ export class GitHubReporter implements Reporter {
|
||||
return link;
|
||||
}
|
||||
|
||||
/**
|
||||
* Fetch ALL PR review (inline) comments, paginating manually like
|
||||
* fetchAllCommentsUncached. Unlike the issue-comments endpoint this one honors
|
||||
* sort/direction, but a full scan is needed anyway (we reconcile every own
|
||||
* thread), so the same oldest-first walk keeps the two fetchers uniform.
|
||||
*/
|
||||
private async fetchAllReviewCommentsUncached(): Promise<ReviewCommentApi[]> {
|
||||
const all: ReviewCommentApi[] = [];
|
||||
const gh = await resolveTrustedTool("gh");
|
||||
for (let page = 1; page <= GitHubReporter.MAX_COMMENT_PAGES; page++) {
|
||||
const { stdout } = await run(
|
||||
gh,
|
||||
[
|
||||
"api",
|
||||
"-X",
|
||||
"GET",
|
||||
`repos/${this.options.repo}/pulls/${this.options.prNumber}/comments`,
|
||||
"-f",
|
||||
"per_page=100",
|
||||
"-f",
|
||||
`page=${page}`,
|
||||
],
|
||||
{ cwd: this.options.cwd },
|
||||
);
|
||||
let batch: ReviewCommentApi[];
|
||||
try {
|
||||
batch = JSON.parse(stdout) as ReviewCommentApi[];
|
||||
} catch {
|
||||
break;
|
||||
}
|
||||
if (!Array.isArray(batch) || batch.length === 0) {
|
||||
break;
|
||||
}
|
||||
all.push(...batch);
|
||||
if (batch.length < 100) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
return all;
|
||||
}
|
||||
|
||||
/** Shared-per-PR, TTL'd review-comments listing (see reviewCommentsByPr). */
|
||||
private fetchAllReviewComments(): Promise<ReviewCommentApi[]> {
|
||||
const key = prAuthorCacheKey(this.options.repo, this.options.prNumber, this.options.cwd);
|
||||
const now = Date.now();
|
||||
const cached = reviewCommentsByPr.get(key);
|
||||
if (cached && now - cached.at <= GitHubReporter.COMMENTS_CACHE_TTL_MS) {
|
||||
return cached.comments;
|
||||
}
|
||||
const comments = this.fetchAllReviewCommentsUncached().catch((error: unknown) => {
|
||||
// Never cache a failure.
|
||||
if (reviewCommentsByPr.get(key)?.comments === comments) {
|
||||
reviewCommentsByPr.delete(key);
|
||||
}
|
||||
throw error;
|
||||
});
|
||||
reviewCommentsByPr.set(key, { at: now, comments });
|
||||
return comments;
|
||||
}
|
||||
|
||||
private invalidateReviewComments(): void {
|
||||
reviewCommentsByPr.delete(
|
||||
prAuthorCacheKey(this.options.repo, this.options.prNumber, this.options.cwd),
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* OUR live inline finding comments under THIS tag: top-level, authored by our
|
||||
* own login (unspoofable), carrying the anchored byte-0 marker. Identity is
|
||||
* author + marker exactly like selectOwnComments; an unresolved login returns
|
||||
* nothing (fail closed). `hasReplies` is derived from the same listed snapshot.
|
||||
*/
|
||||
private async ownInlineComments(): Promise<ExistingInlineComment[]> {
|
||||
const [comments, ownLogin] = await Promise.all([
|
||||
this.fetchAllReviewComments(),
|
||||
this.resolveOwnLogin(),
|
||||
]);
|
||||
if (!ownLogin) {
|
||||
return [];
|
||||
}
|
||||
const repliedTo = new Set(
|
||||
comments
|
||||
.filter((comment) => comment.in_reply_to_id != null)
|
||||
.map((comment) => comment.in_reply_to_id!),
|
||||
);
|
||||
const own: ExistingInlineComment[] = [];
|
||||
for (const comment of comments) {
|
||||
if (comment.in_reply_to_id != null || comment.user?.login !== ownLogin) {
|
||||
continue;
|
||||
}
|
||||
const fp = parseInlineMarkerFp(comment.body ?? "", this.options.commentTag);
|
||||
if (!fp) {
|
||||
continue;
|
||||
}
|
||||
own.push({
|
||||
id: comment.id,
|
||||
fp,
|
||||
body: comment.body ?? "",
|
||||
...(comment.html_url ? { url: comment.html_url } : {}),
|
||||
hasReplies: repliedTo.has(comment.id),
|
||||
});
|
||||
}
|
||||
return own;
|
||||
}
|
||||
|
||||
/** The PR's CURRENT head OID, fetched live — or null. Used to refuse creates
|
||||
* whose commit_id (the head this run reviewed) is no longer the head. */
|
||||
private async liveHeadOid(): Promise<string | null> {
|
||||
try {
|
||||
const gh = await resolveTrustedTool("gh");
|
||||
const { stdout } = await run(
|
||||
gh,
|
||||
[
|
||||
"pr",
|
||||
"view",
|
||||
String(this.options.prNumber),
|
||||
"--repo",
|
||||
this.options.repo,
|
||||
"--json",
|
||||
"headRefOid",
|
||||
"--jq",
|
||||
".headRefOid",
|
||||
],
|
||||
{ cwd: this.options.cwd },
|
||||
);
|
||||
return stdout.trim() || null;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
/** Secondary-rate-limit shapes (403/429): abort the rest of the sync, not just
|
||||
* this mutation — hammering on would dig the hole deeper. */
|
||||
private static isRateLimitError(error: unknown): boolean {
|
||||
return /HTTP 403|HTTP 429|rate limit|secondary rate/i.test(errorMessage(error));
|
||||
}
|
||||
|
||||
// @ref LLP 0008#github-reporter-identity [constrained-by] — inline threads are
|
||||
// patched/deleted only when author + anchored marker both match; creates also require
|
||||
// a resolved own login, or every run would duplicate what it cannot recognize
|
||||
/**
|
||||
* Converge this reporter's inline comments to `targets` (the ACTIVE, in-diff
|
||||
* findings), returning fp → thread permalink for the live full-form threads.
|
||||
* Never throws and never blocks the main comment: every failure degrades to
|
||||
* "that finding renders full-form in the main comment".
|
||||
*
|
||||
* `teardown: false` marks a run whose target set may be INCOMPLETE — a failed
|
||||
* review, a `--scopes` partial run, carried/truncated aggregate state — and makes
|
||||
* the sync additive-only (see planInlineSync): a finding this run cannot see must
|
||||
* not have its thread torn down.
|
||||
*
|
||||
* Creates require the reviewed head (`options.headSha`) to still BE the live head:
|
||||
* after a mid-review push the diff we validated lines against no longer matches
|
||||
* the commit we'd anchor to — a stale commit_id can silently pin the comment to
|
||||
* the wrong code (or 422 wholesale after a force-push) — so creates are skipped
|
||||
* and logged; patches/stubs/deletes are body-only and safe either way.
|
||||
*/
|
||||
private async syncInline(
|
||||
targets: Array<{ finding: Finding; fp: string }>,
|
||||
link: LinkContext,
|
||||
opts: { teardown: boolean },
|
||||
): Promise<Map<string, string>> {
|
||||
const urls = new Map<string, string>();
|
||||
const config = this.options.inline;
|
||||
if (!config?.enabled || !link.diffLines) {
|
||||
return urls;
|
||||
}
|
||||
const eligible = targets.filter(
|
||||
({ finding }) =>
|
||||
finding.line != null && link.diffLines!.get(finding.file)?.has(finding.line) === true,
|
||||
);
|
||||
let existing: ExistingInlineComment[];
|
||||
try {
|
||||
existing = await this.ownInlineComments();
|
||||
if (eligible.length > 0 && (await this.resolveOwnLogin()) == null) {
|
||||
// Without an own login we cannot recognize our previous comments, so a
|
||||
// create now would duplicate on every future run. Fail closed.
|
||||
process.stderr.write(
|
||||
"inline sync: could not resolve the posting identity; skipping inline comments this run.\n",
|
||||
);
|
||||
return urls;
|
||||
}
|
||||
} catch (error) {
|
||||
process.stderr.write(
|
||||
`inline sync: could not list review comments (findings render in the main comment): ${errorMessage(error)}\n`,
|
||||
);
|
||||
return urls;
|
||||
}
|
||||
|
||||
const plan = planInlineSync({
|
||||
targets: eligible,
|
||||
existing,
|
||||
maxComments: config.maxComments,
|
||||
teardown: opts.teardown,
|
||||
});
|
||||
|
||||
// One live-head check per sync, only when creating. A mismatch (or an unknown
|
||||
// reviewed head) skips creates; everything body-only still runs.
|
||||
let creates = plan.create;
|
||||
if (creates.length > 0) {
|
||||
const reviewedHead = this.options.headSha;
|
||||
const live = reviewedHead ? await this.liveHeadOid() : null;
|
||||
if (!reviewedHead || live !== reviewedHead) {
|
||||
process.stderr.write(
|
||||
`inline sync: PR head ${reviewedHead ? "moved during the review" : "is unknown"}; ` +
|
||||
`skipping ${creates.length} new inline comment(s) this run.\n`,
|
||||
);
|
||||
creates = [];
|
||||
}
|
||||
}
|
||||
|
||||
const counts = { created: 0, patched: 0, stubbed: 0, deleted: 0, failed: 0 };
|
||||
let aborted = false;
|
||||
const attempt = async (fn: () => Promise<void>): Promise<void> => {
|
||||
if (aborted) {
|
||||
return;
|
||||
}
|
||||
try {
|
||||
await fn();
|
||||
} catch (error) {
|
||||
counts.failed++;
|
||||
if (GitHubReporter.isRateLimitError(error)) {
|
||||
aborted = true;
|
||||
process.stderr.write(
|
||||
`inline sync: rate-limited; aborting remaining inline mutations this run: ${errorMessage(error)}\n`,
|
||||
);
|
||||
} else {
|
||||
process.stderr.write(`inline sync: mutation failed (skipped): ${errorMessage(error)}\n`);
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
// Sequential on purpose: parallel content creation is what trips GitHub's
|
||||
// secondary rate limit.
|
||||
for (const { comment, finding } of plan.patch) {
|
||||
if (comment.url) {
|
||||
urls.set(comment.fp, comment.url);
|
||||
}
|
||||
const body = renderInlineCommentBody(finding, this.options.commentTag, comment.fp);
|
||||
if (inlineBodiesEqual(comment.body, body)) {
|
||||
continue;
|
||||
}
|
||||
await attempt(async () => {
|
||||
await this.patchReviewComment(comment.id, body);
|
||||
counts.patched++;
|
||||
});
|
||||
}
|
||||
for (const entry of creates) {
|
||||
await attempt(async () => {
|
||||
const created = await this.createReviewComment(
|
||||
renderInlineCommentBody(entry.finding, this.options.commentTag, entry.fp),
|
||||
entry.finding.file,
|
||||
entry.finding.line!,
|
||||
);
|
||||
counts.created++;
|
||||
if (created?.html_url) {
|
||||
urls.set(entry.fp, created.html_url);
|
||||
}
|
||||
});
|
||||
}
|
||||
for (const comment of plan.stub) {
|
||||
const body = inlineStubBody(this.options.commentTag, comment.fp);
|
||||
if (inlineBodiesEqual(comment.body, body)) {
|
||||
continue;
|
||||
}
|
||||
await attempt(async () => {
|
||||
await this.patchReviewComment(comment.id, body);
|
||||
counts.stubbed++;
|
||||
});
|
||||
}
|
||||
for (const comment of plan.remove) {
|
||||
await attempt(async () => {
|
||||
await this.deleteReviewComment(comment.id);
|
||||
counts.deleted++;
|
||||
});
|
||||
}
|
||||
|
||||
const total = counts.created + counts.patched + counts.stubbed + counts.deleted + counts.failed;
|
||||
if (total > 0) {
|
||||
const summary =
|
||||
`inline sync: ${counts.created} created, ${counts.patched} patched, ` +
|
||||
`${counts.stubbed} stubbed, ${counts.deleted} deleted, ${counts.failed} failed`;
|
||||
process.stderr.write(`${summary}\n`);
|
||||
// Inline comments are mutated in place like the main comment, so this line is
|
||||
// the only per-run record of what changed (mirrors upsertComment's mirror).
|
||||
await appendStepSummary(`🤖 AI review — ${summary}`);
|
||||
}
|
||||
return urls;
|
||||
}
|
||||
|
||||
private async createReviewComment(
|
||||
body: string,
|
||||
path_: string,
|
||||
line: number,
|
||||
): Promise<{ html_url?: string } | null> {
|
||||
this.invalidateReviewComments();
|
||||
const gh = await resolveTrustedTool("gh");
|
||||
const payload = JSON.stringify({
|
||||
body,
|
||||
commit_id: this.options.headSha,
|
||||
path: path_,
|
||||
line,
|
||||
side: "RIGHT",
|
||||
});
|
||||
const dir = await mkdtemp(path.join(tmpdir(), "ecr-"));
|
||||
const jsonPath = path.join(dir, "review-comment.json");
|
||||
try {
|
||||
await writeFile(jsonPath, payload, "utf8");
|
||||
const { stdout } = await run(
|
||||
gh,
|
||||
[
|
||||
"api",
|
||||
"-X",
|
||||
"POST",
|
||||
`repos/${this.options.repo}/pulls/${this.options.prNumber}/comments`,
|
||||
"--input",
|
||||
jsonPath,
|
||||
],
|
||||
{ cwd: this.options.cwd },
|
||||
);
|
||||
try {
|
||||
return JSON.parse(stdout) as { html_url?: string };
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
} finally {
|
||||
await rm(dir, { recursive: true, force: true });
|
||||
}
|
||||
}
|
||||
|
||||
private async patchReviewComment(commentId: number, body: string): Promise<void> {
|
||||
this.invalidateReviewComments();
|
||||
const gh = await resolveTrustedTool("gh");
|
||||
await this.withBodyFile(body, (jsonPath) =>
|
||||
run(
|
||||
gh,
|
||||
[
|
||||
"api",
|
||||
"-X",
|
||||
"PATCH",
|
||||
`repos/${this.options.repo}/pulls/comments/${commentId}`,
|
||||
"--input",
|
||||
jsonPath,
|
||||
],
|
||||
{ cwd: this.options.cwd },
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
private async deleteReviewComment(commentId: number): Promise<void> {
|
||||
this.invalidateReviewComments();
|
||||
const gh = await resolveTrustedTool("gh");
|
||||
await run(
|
||||
gh,
|
||||
["api", "-X", "DELETE", `repos/${this.options.repo}/pulls/comments/${commentId}`],
|
||||
{ cwd: this.options.cwd },
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Add or remove per-PR finding dismissals in the reviewer's comment and re-render
|
||||
* it in place — no re-review needed (the comment embeds the full review state). The
|
||||
@@ -841,6 +1521,45 @@ export class GitHubReporter implements Reporter {
|
||||
state.inputHash,
|
||||
);
|
||||
await this.patchComment(existing.id, body);
|
||||
// Additive-only inline updates for exactly the fps this action named — never a
|
||||
// full stale sweep: the embedded state an aggregate stores may be TRUNCATED
|
||||
// (renderAggregateMarkdown trims kept findings to fit GitHub's body cap), so
|
||||
// "not in state" here does not mean "gone" and must not tear a thread down.
|
||||
// Fail-soft: a dismissal must never fail on the inline layer.
|
||||
if (this.options.inline?.enabled) {
|
||||
try {
|
||||
const own = await this.ownInlineComments();
|
||||
const byFp = new Map(own.map((comment) => [comment.fp, comment]));
|
||||
const findingById = stateFindingsById(state);
|
||||
for (const fp of next.matched) {
|
||||
const comment = byFp.get(fp);
|
||||
if (!comment) {
|
||||
continue;
|
||||
}
|
||||
const stub = inlineStubBody(this.options.commentTag, fp);
|
||||
if (!inlineBodiesEqual(comment.body, stub)) {
|
||||
await this.patchReviewComment(comment.id, stub);
|
||||
}
|
||||
}
|
||||
for (const fp of remove) {
|
||||
const comment = byFp.get(fp);
|
||||
const finding = findingById.get(fp);
|
||||
// A finding truncated out of the state (or a thread that was deleted) just
|
||||
// renders full-form in the main comment until the next review re-creates it.
|
||||
if (!comment || !finding) {
|
||||
continue;
|
||||
}
|
||||
const full = renderInlineCommentBody(finding, this.options.commentTag, fp);
|
||||
if (!inlineBodiesEqual(comment.body, full)) {
|
||||
await this.patchReviewComment(comment.id, full);
|
||||
}
|
||||
}
|
||||
} catch (error) {
|
||||
process.stderr.write(
|
||||
`inline sync: dismissal update skipped (main comment already updated): ${errorMessage(error)}\n`,
|
||||
);
|
||||
}
|
||||
}
|
||||
return {
|
||||
dismissedCount: next.dismissed.length,
|
||||
matched: next.matched,
|
||||
|
||||
@@ -160,4 +160,20 @@
|
||||
// "protectedCategories": ["secrets", "security"],
|
||||
// "maxAdjudications": 10 // cap on model calls per run
|
||||
// }
|
||||
|
||||
// Inline PR comments (ROOT-ONLY, off by default). When enabled, each finding
|
||||
// anchored to a line in the PR diff is posted as an inline review comment on
|
||||
// that line (full rationale/suggestion there), and the main comment renders it
|
||||
// short — title, links, a one-line rationale — with a link to the thread. The
|
||||
// main comment stays the single source of truth (state, /dismiss, replies).
|
||||
// Threads update in place across re-reviews; a retired thread with human
|
||||
// replies is stubbed, never deleted. Replies on inline threads are matched to
|
||||
// their finding like main-comment replies (same clearing rules).
|
||||
// NOTE: the first enabled run on a PR sends one notification per created
|
||||
// inline comment — `maxComments` bounds that. Disabling later leaves existing
|
||||
// threads up until a comment-mode switch (or manual cleanup) clears them.
|
||||
// "inline": {
|
||||
// "enabled": false,
|
||||
// "maxComments": 20 // per PR (single comment mode) / per scope (per-scope mode)
|
||||
// }
|
||||
}
|
||||
Reference in new issue
Block a user