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 This PR changes 100-499 lines, ignoring generated files. label Jun 26, 2026
@superagent-security

Copy link
Copy Markdown

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 #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

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant