Skip to content

PLAN-inline-comments.md

Latest commit

 

History

History
230 lines (193 loc) · 11.7 KB

PLAN-inline-comments.md

File metadata and controls

230 lines (193 loc) · 11.7 KB
Raw

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 does not list an inlined finding again: it shows each inlined finding's id, linked to its thread, and its location, in one row per severity. 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 threads. Accepted risk: the PR author can resolve (collapse) an inline thread, and the main comment then shows only its id; the thread stays on the PR and anyone can expand it. Outdated threads (line: null) are detected and listed in full.
  • 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:

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 id rows

  • LinkContext.inlineUrls?: Map<string, string>. NEVER mutate the shared routed-flow link object — render with a copy { ...link, inlineUrls }.
  • renderSeveritySections leaves every finding with an inline URL out of the severity lists. Under 💬 **Inline comments on the diff (n)** it adds one row per severity with each finding's id: linked to its thread, plus a reply link when an author answered. Severity headings count only the findings listed under them. Missing URL ⇒ full form (tested). See LLP 0008 "Inline Threads".
  • 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, main-comment id rows.
  • 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, id rows + 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.