feat: quiet re-runs — cross-run dedup, one status comment, no per-run review blocks#12
Merged
Merged
Conversation
The LLM generates different titles for the same conceptual finding across runs, so the sha256(title)-based anchor key never matched existing comments — every finding was treated as new. Replace with file:category:line keys and ±20-line proximity matching. Old-format (title-hash) anchors are gracefully skipped during parsing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change replaces title-hash anchors with structured line-based anchors, adds proximity duplicate detection, separates bot-comment retrieval APIs, updates finding posting and status handling, and isolates non-trigger comments in workflow concurrency. ChangesStructured rerun deduplication
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
extractAnchors,ANCHOR_PATTERN.execwithout the global flag means only the first anchor in each comment body is parsed; if a body can contain multiple anchors you may want to use a global regex and loop to collect all of them. - In
orchestrate.ts, you re-declare the{ file: string; category: string; line: number }shape forexistingAnchors; consider reusing the exportedAnchorEntrytype fromcomment-mapping.tsto keep the types in sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `extractAnchors`, `ANCHOR_PATTERN.exec` without the global flag means only the first anchor in each comment body is parsed; if a body can contain multiple anchors you may want to use a global regex and loop to collect all of them.
- In `orchestrate.ts`, you re-declare the `{ file: string; category: string; line: number }` shape for `existingAnchors`; consider reusing the exported `AnchorEntry` type from `comment-mapping.ts` to keep the types in sync.
## Individual Comments
### Comment 1
<location path="src/orchestrate.ts" line_range="279" />
<code_context>
// Step 12.5: cross-run dedup
- let existingAnchors: Set<string>
+ let existingAnchors: { file: string; category: string; line: number }[]
try {
const existingComments = await githubClient.fetchReviewComments({
</code_context>
<issue_to_address>
**suggestion:** Use the shared AnchorEntry type instead of duplicating the shape here.
`AnchorEntry` in `comment-mapping.ts` already matches this structure. Please reuse it here (e.g. `let existingAnchors: AnchorEntry[]`) to avoid duplication and ensure consistency if the shape changes later.
Suggested implementation:
```typescript
extractAnchors,
isDuplicateFinding,
mapFindingsToReview,
RERUN_ANCHOR,
type ReviewComment,
type AnchorEntry,
})
```
```typescript
// Step 12.5: cross-run dedup
let existingAnchors: AnchorEntry[]
```
</issue_to_address>
### Comment 2
<location path="src/review/__tests__/comment-mapping.test.ts" line_range="386-395" />
<code_context>
+describe("extractAnchors", () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for multiple anchors per body and malformed/invalid anchors
Current tests cover numeric line parsing, skipping old title-hash anchors, bodies without anchors, and empty input. To strengthen coverage:
1) Add a case where a single body contains multiple anchors to confirm they’re all extracted (or to document that only one is expected).
2) Add cases for malformed anchors (e.g., negative/zero line numbers, non-integer values, missing file/category segments) to ensure they’re ignored and protect the parsing/validation logic from regressions.
Suggested implementation:
```typescript
describe("extractAnchors", () => {
it("parses line-based anchor keys from comment bodies", () => {
const bodies = [
"Some comment body\n\n<!-- umm-actually:src/a.ts:correctness:42 -->",
"Another body\n\n<!-- umm-actually:src/b.ts:security:100 -->",
]
const anchors = extractAnchors(bodies)
expect(anchors).toHaveLength(2)
})
it("parses multiple anchors from a single body", () => {
const bodies = [
[
"Some comment body",
"<!-- umm-actually:src/a.ts:correctness:42 -->",
"Some more content",
"<!-- umm-actually:src/a.ts:style:100 -->",
].join("\n"),
]
const anchors = extractAnchors(bodies)
expect(anchors).toHaveLength(2)
})
it("ignores malformed or invalid anchors", () => {
const bodies = [
// negative line number
"Body\n\n<!-- umm-actually:src/a.ts:correctness:-1 -->",
// zero line number
"Body\n\n<!-- umm-actually:src/a.ts:correctness:0 -->",
// non-integer line number
"Body\n\n<!-- umm-actually:src/a.ts:correctness:not-a-number -->",
// missing file segment
"Body\n\n<!-- umm-actually::correctness:42 -->",
// missing category segment
"Body\n\n<!-- umm-actually:src/a.ts::42 -->",
// completely unrelated HTML comment
"Body\n\n<!-- some other comment -->",
]
const anchors = extractAnchors(bodies)
expect(anchors).toHaveLength(0)
})
```
These tests assume that `extractAnchors` returns an array-like collection and that invalid/malformed anchors are dropped rather than throwing. If `extractAnchors` instead returns a map or object, or represents anchors differently, you should update the `expect(anchors).toHaveLength(...)` assertions to match the actual structure (e.g., `Object.keys(anchors).length`, `anchors.size`, etc.), and optionally assert more specific properties (file path, category, line) to tighten coverage.
</issue_to_address>
### Comment 3
<location path="src/__tests__/orchestrate.test.ts" line_range="891" />
<code_context>
{
path: "src/a.ts",
- body: "body\n\n<!-- umm-actually:src/a.ts:correctness:aaaaaaaa -->",
+ body: "body\n\n<!-- umm-actually:src/a.ts:correctness:42 -->",
},
],
</code_context>
<issue_to_address>
**suggestion (testing):** Add orchestrate-level tests for old-format anchors and line-proximity dedup behavior
This change keeps the test green but no longer covers the new behaviors:
1) Legacy title-hash anchors should be ignored by `extractAnchors`, so a rerun with only `...:aaaaaaaa`-style anchors should behave like a first run (no dedup). Please add an orchestrate test that feeds `fetchReviewComments` with such legacy anchors and asserts that findings are *not* deduped.
2) Cross-run dedup is now proximity-based. Add a test with an existing anchor at line N and a new finding at N±k (within 20 lines) and assert they dedup, plus another just beyond the threshold that asserts a new comment is posted, to verify orchestrate matches the new `isDuplicateFinding` semantics end to end.
Suggested implementation:
```typescript
// This mock uses a legacy title-hash anchor (aaaaaaaa) to ensure
// extractAnchors ignores it and orchestrate does not dedup based on it.
fetchReviewComments: async () => [
{
path: "src/a.ts",
body: "body\n\n<!-- umm-actually:src/a.ts:correctness:aaaaaaaa -->",
},
],
upsertSummaryComment: async () => {
```
To fully implement your suggestion, the following additional changes are needed elsewhere in `src/__tests__/orchestrate.test.ts`:
1. **Legacy anchor / no-dedup orchestrate test**
Add a new test (or adapt the one that uses the above `fetchReviewComments` mock) roughly like:
```ts
it("does not dedup against legacy title-hash anchors", async () => {
const github = {
fetchReviewComments: async () => [
{
path: "src/a.ts",
// legacy anchor: umm-actually:src/a.ts:correctness:aaaaaaaa
body: "body\n\n<!-- umm-actually:src/a.ts:correctness:aaaaaaaa -->",
},
],
createReviewComment: jest.fn(),
// ...any other methods required by orchestrate...
};
await orchestrate({
github,
findings: [
{
path: "src/a.ts",
line: 10,
ruleId: "correctness",
// ...other required finding fields...
},
],
// ...other orchestrate options...
});
// Assert that a new comment is still posted (no cross-run dedup)
expect(github.createReviewComment).toHaveBeenCalled();
});
```
This ensures that `extractAnchors` ignores legacy `aaaaaaaa` anchors and that orchestrate behaves like a first run when only those anchors exist.
2. **Proximity-based cross-run dedup tests**
Add two orchestrate-level tests that verify proximity-based dedup semantics end-to-end:
a. **Within-threshold dedup (N±k, k ≤ 20):**
```ts
it("dedups findings within 20 lines of an existing anchor", async () => {
const github = {
fetchReviewComments: async () => [
{
path: "src/a.ts",
// existing anchor at line 100
body: "body\n\n<!-- umm-actually:src/a.ts:correctness:100 -->",
},
],
createReviewComment: jest.fn(),
// ...any other methods required by orchestrate...
};
await orchestrate({
github,
findings: [
{
path: "src/a.ts",
line: 110, // within +10 lines of existing anchor
ruleId: "correctness",
// ...other required finding fields...
},
],
// ...other orchestrate options...
});
// No new comment should be created; orchestrate should dedup
expect(github.createReviewComment).not.toHaveBeenCalled();
});
```
b. **Beyond-threshold no-dedup (N±k, k > 20):**
```ts
it("posts a new comment when finding is more than 20 lines from existing anchor", async () => {
const github = {
fetchReviewComments: async () => [
{
path: "src/a.ts",
// existing anchor at line 100
body: "body\n\n<!-- umm-actually:src/a.ts:correctness:100 -->",
},
],
createReviewComment: jest.fn(),
// ...any other methods required by orchestrate...
};
await orchestrate({
github,
findings: [
{
path: "src/a.ts",
line: 121, // just beyond the 20-line threshold
ruleId: "correctness",
// ...other required finding fields...
},
],
// ...other orchestrate options...
});
// A new comment should be created because it's beyond the proximity threshold
expect(github.createReviewComment).toHaveBeenCalled();
});
```
3. **Align with existing test helpers and conventions**
- If your test file already has helpers for building `github` clients or `findings`, reuse them instead of inlining objects as in the examples above.
- Ensure `orchestrate` is imported the same way as in the existing tests.
- If the existing tests assert on `upsertSummaryComment` or other methods instead of `createReviewComment`, adapt the expectations to match your current orchestrate behavior.
These additional changes will provide orchestrate-level coverage for:
- Ignoring legacy title-hash anchors (`...:aaaaaaaa`) and avoiding cross-run dedup based on them.
- Proximity-based dedup semantics (within vs beyond 20 lines) matching `isDuplicateFinding` end to end.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/orchestrate.ts (1)
279-297: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid
let; prefer immutable data.The variable
existingAnchorsis declared usingletto accommodate assignment inside thetry/catchblock. As per coding guidelines, prefer immutable data and avoidlet. You can chain.catch()to achieve an immutable assignment.♻️ Proposed refactor
- let existingAnchors: { file: string; category: string; line: number }[] - try { - const existingComments = await githubClient.fetchReviewComments({ - prNumber: prContext.prNumber, - }) - existingAnchors = extractAnchors( - existingComments.map((comment) => comment.body), - ) - } catch (fetchError) { - const errorDetail = - fetchError instanceof Error - ? `[${fetchError.name}]: ${fetchError.message}` - : String(fetchError) - logger.warn("failed to fetch existing comments — treating as first run", { - error: errorDetail, - }) - existingAnchors = [] - } + const existingComments = await githubClient + .fetchReviewComments({ prNumber: prContext.prNumber }) + .catch((fetchError) => { + const errorDetail = + fetchError instanceof Error + ? `[${fetchError.name}]: ${fetchError.message}` + : String(fetchError) + logger.warn("failed to fetch existing comments — treating as first run", { + error: errorDetail, + }) + return [] + }) + + const existingAnchors = extractAnchors( + existingComments.map((comment) => comment.body), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/orchestrate.ts` around lines 279 - 297, Refactor the existingAnchors initialization to use an immutable const assignment by chaining catch handling onto the fetchReviewComments/extractAnchors promise. Preserve the current warning details and fallback to an empty anchor list when fetching fails, while removing the separate declaration and try/catch assignment.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/orchestrate.ts`:
- Around line 298-302: Rename the filter callback parameter f in the newFindings
calculation to an explicit name such as finding, and update its use in
isDuplicateFinding without changing the filtering behavior.
In `@src/review/comment-mapping.ts`:
- Around line 72-81: Update isDuplicateFinding to use an explicit anchor
parameter name, convert its multiline arrow-function expression into a block
body with an explicit return, and wrap the multiline anchors.some predicate in a
block body returning the boolean condition.
- Around line 48-69: Refactor extractAnchors to avoid mutating the entries
accumulator inside the loop by using flatMap or an equivalent immutable
transformation over commentBodies. Preserve all existing filtering, anchor
parsing, and AnchorEntry construction behavior, including skipping invalid
matches and line values.
---
Outside diff comments:
In `@src/orchestrate.ts`:
- Around line 279-297: Refactor the existingAnchors initialization to use an
immutable const assignment by chaining catch handling onto the
fetchReviewComments/extractAnchors promise. Preserve the current warning details
and fallback to an empty anchor list when fetching fails, while removing the
separate declaration and try/catch assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ae2fe0f-9eb0-4be3-8ab1-64ba5d690421
📒 Files selected for processing (4)
src/__tests__/orchestrate.test.tssrc/orchestrate.tssrc/review/__tests__/comment-mapping.test.tssrc/review/comment-mapping.ts
…ions Re-run detection previously inferred from inline anchors, which zero-findings and body-only first runs never leave behind — every subsequent run re-detected "first run" and posted a brand-new review instead of updating the summary comment. Detection now asks GitHub whether the bot already posted any review on the PR (hasPriorBotReview, reusing the viewer-login identity discovery), so re-runs always route to the summary upsert regardless of what earlier runs posted. Dedup positioning: fetchReviewComments now returns GitHub's live line (kept updated across pushes) plus original_line; extractAnchors prefers the live position over the line embedded at post time, so dedup follows code motion. With motion handled, LINE_PROXIMITY shrinks 20 → 5 — a wide window silently suppresses genuinely new findings in the same file+category, while a narrow one merely risks a visible duplicate. Also: dedup runs before the findings cap so duplicates don't consume slots, re-run reviews now report droppedByCap, and review-bot feedback addressed (AnchorEntry reuse, explicit callback names, immutable extractAnchors via flatMap). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Memoize the viewer GraphQL query inside the client factory so requestBotReview and hasPriorBotReview share the result instead of hitting the API twice per run. Replace the remaining inline error formatting in orchestrate step 3.5 with the describeError helper introduced by this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace === undefined / !== undefined guards with truthy/falsy checks across comment-mapping.ts and orchestrate.ts, per project convention. All guarded values are objects or positive integers where 0/false/"" are never valid. Also simplify .filter(section !== "") to .filter(Boolean). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![umm-actually[bot]](https://avatars.githubusercontent.com/in/4233716?s=60&v=4){kind=small}
…d tighten pagination assertions Add missing test coverage for the Phase 1 memoization of resolveBotLogin (verifies only one viewer query across requestBotReview + hasPriorBotReview) and the hasPriorBotReview page cap warning (mirrors the existing fetchReviewComments cap test). Tighten the pagination test from toMatchObject to full toEqual per project assertion conventions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/github/client.ts (1)
46-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
listReviewsfrom the official Octokit type.Adding another hand-written SDK signature risks drifting from
@actions/github. Preserve the narrow injectable surface, but derive this method’s parameters and return type from its Octokit client type.As per coding guidelines: “Use official SDKs” and “prefer SDK-provided types over redefined shapes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/github/client.ts` around lines 46 - 52, Update the listReviews declaration to derive its parameter and return types from the official Octokit client type exposed by `@actions/github`, while preserving the existing narrow injectable method surface. Remove the hand-written params object and Promise<{ data: unknown }> shape, and reuse the SDK’s listReviews type definitions.Source: Coding guidelines
src/orchestrate.ts (1)
124-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
(params, logger)for these I/O helpers.Both helpers have three positional arguments. Move
githubClientandprNumberinto a named parameter object and retainloggeras the required second argument.As per coding guidelines: “Use named parameter objects for functions with more than two arguments” and “Data-layer and I/O functions must accept
(params, logger).”Also applies to: 141-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/orchestrate.ts` around lines 124 - 128, Update detectRerun and the other referenced I/O helper to accept a named params object containing githubClient and prNumber, followed by the required logger argument. Update every call site to pass (params, logger) while preserving the existing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/orchestrate.test.ts`:
- Around line 925-928: Update the assertions in the orchestrate test cases
around the findings-count checks to compare the deterministic submitReviewCalls
payload with the exact expected findings for each scenario. Keep the count
assertions if useful, but ensure each case verifies the complete submitted
review value so removing the wrong finding or submitting an incomplete payload
cannot pass.
In `@src/github/__tests__/client.test.ts`:
- Around line 771-789: Strengthen the tests for hasPriorBotReview by asserting
the exact listReviewsCalls in both the “no review” test and the corresponding
test around the memoization scenario. Verify each call uses the expected
pull-request number and review-listing parameters, ensuring the tests fail if
hasPriorBotReview skips listing reviews while preserving the existing result
assertions.
In `@src/github/client.ts`:
- Around line 132-137: Extend the review-comment schema to include the nested
user.login field, then update fetchReviewComments to retain only comments
authored by the bot before returning them to extractAnchors. Preserve the
existing anchor parsing behavior for bot-authored comments.
---
Nitpick comments:
In `@src/github/client.ts`:
- Around line 46-52: Update the listReviews declaration to derive its parameter
and return types from the official Octokit client type exposed by
`@actions/github`, while preserving the existing narrow injectable method surface.
Remove the hand-written params object and Promise<{ data: unknown }> shape, and
reuse the SDK’s listReviews type definitions.
In `@src/orchestrate.ts`:
- Around line 124-128: Update detectRerun and the other referenced I/O helper to
accept a named params object containing githubClient and prNumber, followed by
the required logger argument. Update every call site to pass (params, logger)
while preserving the existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d4a71de-9ef5-4d32-8aa4-1a602b25a80c
📒 Files selected for processing (6)
src/__tests__/orchestrate.test.tssrc/github/__tests__/client.test.tssrc/github/client.tssrc/orchestrate.tssrc/review/__tests__/comment-mapping.test.tssrc/review/comment-mapping.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/review/tests/comment-mapping.test.ts
… comments
The dogfooded self-review runs on this PR exposed a live bug: GraphQL
viewer{login} on an app installation token already returns the bot user
(`<slug>[bot]`), so unconditionally appending the suffix built
`<slug>[bot][bot]` — a nonexistent user. That 404ed requestBotReview
(silently broken since #11) and made hasPriorBotReview never match its
own reviews, so re-runs took the first-run path and the summary comment
never appeared. Append the suffix only when absent.
Also, per CodeRabbit's security finding: fetchReviewComments now keeps
only the bot's own comments — anyone could paste an
`<!-- umm-actually:... -->` marker into a review comment and silently
suppress a real future finding at that location.
Plus review feedback: rerun summary wording now claims exactly what
totalCount counts (tracked inline findings — body-only and legacy
findings aren't parseable), detectRerun/fetchExistingAnchors take
(params, logger) per convention, exact whole-payload asserts on re-run
submitReview calls, and listReviewsCalls proofs so hasPriorBotReview
can't silently no-op.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Addressing the two CodeRabbit nitpicks from the review body, plus one beyond-diff discovery:
🔍 ship-check · pr-monitor · claude-fable-5 |
Any PR comment without the @umm review trigger phrase still creates a workflow run that joins the self-review concurrency group before the job guard skips it — and cancel-in-progress kills the legitimate review run already executing. Observed live: the pipeline's own reply comments canceled the self-review of the commit they were replying about. Route non-trigger comment runs into a per-run noop group instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The method filters to the bot's own comments since the anchor-spoofing fix — the name now says so. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
aliasunder
changed the title
GitHub's live `line` on a multi-line review comment is the END of the range, while anchors embed the finding's `line` — the START. A wide finding (span > LINE_PROXIMITY) compared by its end line blew past the window and re-posted as a duplicate. Prefer start_line / original_start_line when present. Found by the bot reviewing its own dedup code on this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A PAT resolves viewer to a User whose bare login authors everything the action posts — appending [bot] would break author matching for PAT users. Query __typename and suffix only Bot viewers (still guarding against the already-suffixed installation-token form). Found by the bot's re-review of its own identity code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the lastIndexOf/slice index surgery in parseAnchorKey with a named-groups pattern — the regex states the key shape (file may contain colons, category never does, line is a positive integer) instead of making the reader simulate string offsets. Positivity moves into the pattern, dropping the separate integer check. Behavior unchanged; the malformed-key test table passes as-is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mment Every run now walks the same path; the first-run/re-run branch is gone. Dedup sources (the bot's inline comments with live positions, plus its beyond-diff issue comments by anchor line) are naturally empty on a first run, and the status comment's existence only picks the wording. Findings post without narrative wrappers: anchorable ones batch into a single review whose body is an invisible HTML marker (one notification, a bare "reviewed" timeline event, no prose blocks stacking per run); beyond-diff findings post as individual issue comments so each new finding is a visible event to PR watchers. All narration lives in the one always-upserted status comment — the run receipt, posted or updated on every completed run, findings or not. Deleted: hasPriorBotReview, detectRerun, the findings review body, the zero-findings review, buildReviewBody/buildRerunSummary and the 422 body-only fallback (anchor-rejected findings re-route to issue comments; other post failures self-heal — unposted findings carry no anchor, so the next run re-reports them). submitReview survives for skip notices only. Status anchor renamed to umm-actually-status. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
aliasunder
changed the title
|
Caution Review failedAn error occurred during the review process. Please try again later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
umm-actually re-reviewed at 2 new finding(s) posted (13 tracked finding(s) across all runs). umm-actually · deepseek/deepseek-v4-pro |
When a findings post fails (non-422), the findings stay unposted and self-heal next run — but the status comment claimed they were posted. Counts now come from the actual post outcomes, with an explicit "N finding(s) could not be posted — they will re-report on the next run" note, and the findings_count output follows suit. Found by the bot's first review under the new architecture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
upsertSummaryComment matched any comment containing the status anchor — a pasted marker from another author would hijack the update target (and the edit would 403; bots can't modify other users' comments). Match only the bot's own comments, same rule as the dedup sources. Found by the bot reviewing its own upsert. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/github/__tests__/client.test.ts`:
- Around line 983-1027: Update the two pagination tests around
fetchBotIssueComments to assert the complete listCommentsCalls values, including
page numbers 1 and 2 for the author-filter case and pages 1 through 10 for the
page-cap case. Preserve the existing response and warning assertions while using
exact whole-call parameter comparisons to catch repeated page: 1 requests.
In `@src/github/client.ts`:
- Around line 487-490: Restrict status-comment detection to bodies beginning
with the marker: update the matching logic in src/github/client.ts lines 487-490
and src/orchestrate.ts lines 116-118 to use startsWith with anchor or
STATUS_ANCHOR. Add coverage in src/github/__tests__/client.test.ts lines
1269-1296 proving a bot finding containing the marker later is not updated, and
in src/__tests__/orchestrate.test.ts lines 1121-1138 proving it does not
classify the first run as a rerun.
In `@src/orchestrate.ts`:
- Around line 425-432: Update the status payload built around buildStatusComment
so totalCount does not treat duplicate comment anchors as distinct findings:
coalesce existingAnchors using the same proximity rule before counting them,
while preserving postedCount handling and the existing status fields. If
deduplication is not available in this flow, relabel the displayed count as
tracked comment anchors instead of findings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a8ed2b17-15f1-488f-84c1-31346d2d0f5c
📒 Files selected for processing (7)
.github/workflows/self_review.ymlsrc/__tests__/orchestrate.test.tssrc/github/__tests__/client.test.tssrc/github/client.tssrc/orchestrate.tssrc/review/__tests__/comment-mapping.test.tssrc/review/comment-mapping.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/review/comment-mapping.ts
- Match the status marker only at the comment start (startsWith over includes) in both the upsert target search and the first-run detection — model-generated finding text quoting the marker mid-body could hijack the upsert or misclassify the run as a re-run. - Coalesce existing anchors by the proximity rule before counting: a fail-open repost leaves two anchors for one finding, and the tracked count reports findings, not comment anchors. - Assert exact page sequences in pagination tests — the stub serves responses by call count, so length checks passed even if page 1 were requested repeatedly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
[low/correctness] Action output findings_count excludes unposted beyond-diff findings (confidence: medium)
When Failure scenario: Run generates 1 inline finding and 1 beyond-diff finding. Inline review posts successfully, but the beyond-diff post fails (network error). Suggested fixClarify in the action.yml output description that `findings_count` reflects successfully posted findings, not total generated. Alternatively, add a separate `generated_findings_count` output for observability. |
- Reject file values that could break out of the HTML-comment anchor
("-->", newlines) at the schema boundary, where the structured-output
retry ladder tells the model what to fix. Enforced via refine so the
constraint stays out of the JSON schema sent to OpenRouter.
- Extract only a trailing anchor: the genuine anchor is always the last
thing the bot appends, so an anchor-shaped string quoted in model text
can no longer poison dedup and suppress findings at arbitrary spots.
- Clarify the findings_count output description: successfully posted this
run, not generated — failed posts re-report next run.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Re the beyond-diff finding on |
Residue of this PR's removal of the 422 body-only fallback — submitReview now only posts skip notices and the field was hardcoded false, never read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Re-runs of the review bot were re-posting duplicate findings and stacking a new summary-bearing review block on the PR timeline for every push. This PR reworks both the dedup and the posting architecture so that after any number of runs, the PR shows: findings pinned inline at their defect sites, one issue comment per beyond-diff finding, and exactly one status comment that updates in place.
Architecture (single path, every run)
hasPriorBotReview/first-run-vs-re-run branch is deleted.file:category:lineanchors, ±5-line proximity window, live positions preferred over post-time lines so code motion doesn't consume window width. Runs before themax_findingscap.reviewed / re-reviewed at <sha> — N new finding(s) (M tracked across all runs), plus the cap note. Posted or updated on every completed run, findings or not.Failure posture
Security / identity
<!-- umm-actually:... -->marker from anyone else can't suppress findings (CodeRabbit finding).resolveBotLoginsuffixes[bot]only for Bot viewers and only when absent — fixes theumm-actually[bot][bot]404 that silently brokerequestBotReviewsince feat: request bot as PR reviewer via GraphQL botIds #11, and works under PATs.CI
self_review.yml: non-trigger comments no longer cancel in-flight review runs (guard-skipped runs get a per-run concurrency group).Verification
umm-actually-statusanchor, no summary prose in the timeline🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes