Skip to content

feat(review): post quiet inline review comments on the PR diff - #1528

Merged
JSONbored merged 1 commit into
mainfrom
claude/inline-comments-wire
Jun 26, 2026
Merged

feat(review): post quiet inline review comments on the PR diff#1528
JSONbored merged 1 commit into
mainfrom
claude/inline-comments-wire

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

PR B of 2 — activates the CodeRabbit-style quiet inline-comments feature (PR A #1525 shipped the inert contract). On top of the decision summary, the AI reviewer's line-anchored findings post as one non-blocking review (event: COMMENT, never REQUEST_CHANGES) — a contributor sees exactly what to fix for a resubmission, and the gate is never affected.

  • createPullRequestReviewComments — a quiet COMMENT-event review carrying inline comments, threading the action mode (so a dry-run instance suppresses the write).
  • inline-comments.tsrightSideLinesFromPatch validates each finding's line against the diff hunks (out-of-diff lines are dropped → never a 422); selectInlineComments dedupes + caps; postInlineReviewComments is fail-safe (swallows API errors, never touches the gate); maybePostInlineComments posts once per head SHA — a cache hit carries no findings, so the ~2-min re-gate sweep never reposts, and the off-path does zero extra work (not even a PR-files read).
  • Three-layer gate, all default OFF: GITTENSORY_REVIEW_INLINE_COMMENTS + the GITTENSORY_REVIEW_REPOS allowlist + the per-repo .gittensory.yml review.inline_comments toggle.
  • The decision summary / unified comment is byte-identical — inline comments are a separate, additive review surface.

Scope

  • src/review/inline-comments.ts (new), src/github/pr-actions.ts, src/queue/processors.ts
  • flag plumbing: src/env.d.ts, wrangler.jsonc, worker-configuration.d.ts (cf-typegen)
  • docs: review-configuration.md, self-hosting.md, .env.example
  • tests: inline-comments (new), github-pr-actions

Validation

  • npm run typecheck clean · npm run test:coverage 4641 passed · 100% line+branch on the diff (inline-comments.ts, the new GitHub method, and the processor wiring all fully covered; remaining flags in the touched files are pre-existing) · ui:openapi:check clean
  • Adversarially reviewed (4 verifiers): cache double-post, byte-identical-when-off, and the decision-summary-unchanged invariants confirmed; the review caught one real off-by-one (a trailing-newline patch element added a spurious commentable line → potential 422) — fixed + regression-tested here.

Safety

  • No secrets / private terms. event: COMMENT is hardcoded (can't be a blocking review). Default-OFF at every layer; fully fail-safe (no inline-post failure can affect the gate).

PR B activates the CodeRabbit-style inline-comments feature (PR A #1525 shipped the inert
contract). On top of the decision summary, the AI reviewer's line-anchored findings are
posted as ONE non-blocking review (event: COMMENT, never REQUEST_CHANGES) so a contributor
sees exactly what to fix for a resubmission without the gate ever changing.

- createPullRequestReviewComments: a quiet COMMENT-event review with inline comments,
  threading the action mode (dry-run suppression).
- inline-comments.ts: rightSideLinesFromPatch validates each finding's line against the
  diff hunks (out-of-diff lines dropped → never a 422), selectInlineComments dedupes/caps,
  postInlineReviewComments is fail-safe (swallows API errors, never touches the gate), and
  maybePostInlineComments posts once per head SHA — a cache hit carries no findings, so the
  ~2-min re-gate sweep never reposts, and the off-path does zero extra work.
- Gated at three layers (all default OFF): GITTENSORY_REVIEW_INLINE_COMMENTS, the
  GITTENSORY_REVIEW_REPOS allowlist, and the per-repo review.inline_comments manifest toggle.
- Docs + .env.example + flag plumbing (env.d.ts, wrangler.jsonc, cf-typegen).
@dosubot dosubot Bot added the size:L label Jun 26, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.47%. Comparing base (e21adb2) to head (d24db11).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1528      +/-   ##
==========================================
+ Coverage   95.46%   95.47%   +0.01%     
==========================================
  Files         195      196       +1     
  Lines       21148    21197      +49     
  Branches     7649     7662      +13     
==========================================
+ Hits        20188    20237      +49     
  Misses        383      383              
  Partials      577      577              
Files with missing lines Coverage Δ
src/github/pr-actions.ts 98.68% <100.00%> (+0.09%) ⬆️
src/queue/processors.ts 88.43% <100.00%> (+<0.01%) ⬆️
src/review/inline-comments.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit 7c7a37e into main Jun 26, 2026
18 checks passed
@JSONbored
JSONbored deleted the claude/inline-comments-wire branch June 26, 2026 13:53
andriypolanski pushed a commit to andriypolanski/gittensory that referenced this pull request Jun 27, 2026
…lapse nits (JSONbored#1094)

Soak finding: the bot FALSE-CLOSED a good contributor PR (metagraphed JSONbored#1528,
green CI, mergeable) on a hallucinated blocker — it claimed loadArtifactData was
'not imported -> ReferenceError', but that function is defined locally and used by
~24 call sites. Root cause: the model sees only the DIFF, not the whole file, so
new code referencing an existing (out-of-diff) symbol looks undefined; both free
models made the same mistake -> consensus -> close. (Full-file grounding is enabled
but did not prevent it, so a prompt-level guard is the reliable fix.)

- DIFF SCOPE guard in the review prompt: never report a missing import / undefined
  symbol / 'X not defined -> ReferenceError' as a blocker unless the diff ITSELF
  removes the definition or introduces the symbol without defining it; otherwise it
  is at most a nit. Directly kills the false-close-on-hallucination class.
- Nits now render inside a collapsed <details> toggle (assessment + blockers stay
  visible), per request — keeps the comment focused.
andriypolanski pushed a commit to andriypolanski/gittensory that referenced this pull request Jun 27, 2026
…rdening (JSONbored#1099)

* fix(guardrail): fail-closed on KV outage + broaden crucial-path coverage

Flood-readiness (emission weights rose on metagraphed + gittensory; a contributor
wave is imminent). Two guardrail hardenings:

1. Fail-CLOSED on a KV read fault. loadHardGuardrailGlobs previously fell back to the
   narrow default (.github/workflows + scripts) on a THROWN read, so a KV outage
   correlated with a flood would silently shrink the guarded surface and let
   crown-jewel edits (scoring/auth/rules/the gate) auto-merge. A thrown read now
   returns FAIL_CLOSED_GUARDRAIL_GLOBS = ['**'] → every PR is held for human review
   until the read recovers. A legitimately-absent key still uses the narrow default
   so a freshly-installed repo can operate.

2. Broadened the LIVE per-repo KV globs (done out-of-band, effective immediately):
   - gittensory: added src/upstream/**, src/settings/**, src/review/**,
     src/services/**, src/github/**, src/config/** — the gate/decision/reviewer/
     scoring/auth engine that lived OUTSIDE the old dir-prefix guards (e.g.
     score-breakdown.ts, command-authorization.ts, agent-actions.ts, ai-review.ts,
     ruleset.ts). This is the awesome-claude JSONbored#4196 incident class.
   - metagraphed: added src/** (the old config guarded specific .mjs files but NOT
     src/mcp-server.mjs — the file PR JSONbored#1528 touched).

Tests: change-guardrail asserts the crucial out-of-dir files now hit the guardrail
while infra/data/registry/docs/tests still auto-merge; guardrail-config asserts the
outage path fails closed. Full suite green (3489).

* fix(review): break the Gittensory Gate self-deadlock that froze reviews under load

ROOT CAUSE of the stall (diagnosed from the LIVE audit_events ledger, not the frozen
review_targets/review_audit): the Gittensory Gate is a REQUIRED check, and
fetchLiveCiAggregate counted it like any other CI. A review posts the gate as
in_progress, then prReadyForReview waits for ALL required CI to finish — but the gate
never finishes (it's only concluded by the very review being deferred). So every
green-CI PR deferred forever as 'CI still running' (38 review_deferred_ci_pending in 30
min; metagraphed JSONbored#1532 deferred 4x with all checks green).

Fix: fetchLiveCiAggregate now SKIPS the bot's own GITTENSORY_GATE_CHECK_NAME in both the
check-runs and commit-status loops, so the bot waits on real CI only and never on its
own gate. Test: a green 'test' check + an in_progress 'Gittensory Gate' (gate in the
required set) now reports ciState 'passed', not 'pending'.
andriypolanski pushed a commit to andriypolanski/gittensory that referenced this pull request Jun 27, 2026
…g (#hold-crucial-on-reject) (JSONbored#1109)

Operator decision: a guarded/crucial PR (CI, the review engine, visual) must NEVER be auto-closed, even on a
reject verdict — a hallucinated reject (the JSONbored#1528 near-miss: diff-only false-positives) must not auto-close a
good crucial PR. The owner verifies + closes/merges. The BULK (non-guarded) contributor PRs still auto-close
one-shot on a bad verdict / conflict; only the small crucial set is held. Restores the guardrail check on
willClose (reverts JSONbored#1106's removal). Owner/automation PRs never close regardless.
andriypolanski pushed a commit to andriypolanski/gittensory that referenced this pull request Jun 27, 2026
…er + brace-aware JSON parse (#accuracy-gap-1, #accuracy-gap-3) (JSONbored#1110)

Two reviewbot-parity fixes from the accuracy-gap audit, targeting the metagraphed JSONbored#1528 class
(model hallucinates 'missing import / undefined symbol' on code it cannot see).

GAP-1 (the JSONbored#1528 root): buildAiReviewDiff was a blind 60k head-slice that break-DROPPED whole files
in stored order — no source-first ordering, no hunk-aware reduction. On a multi-file PR the file that
DEFINES a symbol could be dropped while another referenced it, so the reviewer 'saw' an undefined
symbol (survived even with full-file grounding on). Port reviewbot src/core/diff.ts into a new guarded
src/review/review-diff.ts: order SOURCE first (lockfiles/generated/docs/tests drop first), reduce
oversized patches hunk-aware (keep highest-signal hunks) instead of dropping, always list patch-less/
over-budget files with counts, and raise the budget 60k->80k (the 120B models are 128k-context).

GAP-3: parseModelReview used a greedy /{[\s\S]*\}/ that spanned first-{ to last-} and swallowed the
gpt-oss/nemotron <think> scratchpad object together with the verdict, silently corrupting/dropping
reviews. Port reviewbot's brace-depth-aware, string-safe extractLastJsonObject (returns the LAST
complete top-level object).

Full suite green (3499). New: review-diff.test.ts (source survives over lockfiles under tight budget;
patch-less file listed; hunk-aware reduction) + ai-review.test.ts scratchpad+fenced regression tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant