Review engine: multi-pass pipeline, publishing, tool-backed evidence#191
Review engine: multi-pass pipeline, publishing, tool-backed evidence#191
Conversation
…d rule registry Adds multi-pass review pipeline (diff-risk, cross-file-impact, checks-and-tests) with finding deduplication and adjudication scoring. Introduces reviewContextBuilder for structured prompt assembly, reviewRuleRegistry for pass-specific rule overlays, and publication budget controls. Extends review findings schema with finding_class, originating_passes, and adjudication metadata. Updates ReviewPage UI with expanded findings display and PrDetailPane issue resolver improvements. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Introduces an autonomous PR-to-merge driver that runs automate → finalize once, then polls CI and review comments on a self-paced 12-min cadence, fixing valid comments and failing tests in place. Prefers TeamCreate agent teams when available, falls back to parallel Agent calls otherwise. Opens the PR via `ade prs create` when possible so it shows up in ADE's PR tracking; falls back to `gh pr create` only after the agent has genuinely exhausted the ADE path via `--help`-driven discovery. Also narrows /automate to run only the new and affected tests (not the full suite), and makes /finalize's 8-shard parallel run explicit so shards don't get chained serially. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 68ef71a)
Lane worktrees use symlinks named node_modules pointing at the main checkout's installed copies. The previous `node_modules/` pattern only matched directories, so git saw the symlinks as untracked — which in turn blocked `ade prs create` preflight and forced /shipLane to fall back to `gh pr create`. Without the slash, the pattern matches both directories (main checkout) and symlinks (worktrees). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 07623ee)
The 8-shard vitest run, parallel tsup builds, and typecheck fans sometimes leave worker pools behind after the phase exits. They don't fail CI but accumulate across runs and can hold file locks. New Phase 3j kills them scoped to apps/ paths so vitest instances the user may have running elsewhere aren't affected. Adds a Cleanup line to the Phase 4 summary and the completion checklist. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 3a5fbe5)
# Conflicts: # .claude/commands/finalize.md # .claude/commands/shipLane.md # apps/desktop/src/main/main.ts # apps/desktop/src/main/services/ipc/registerIpc.ts # apps/desktop/src/main/services/prs/prService.ts # apps/desktop/src/main/services/pty/ptyService.test.ts # apps/desktop/src/main/services/pty/ptyService.ts # apps/desktop/src/preload/global.d.ts # apps/desktop/src/preload/preload.ts # apps/desktop/src/renderer/browserMock.ts # apps/desktop/src/renderer/components/run/AddCommandDialog.tsx # apps/desktop/src/shared/ipc.ts # apps/desktop/src/shared/types/prs.ts # docs/playbooks/ship-lane.md
…o review - Feedback loop: per-finding acknowledge/dismiss/snooze/suppress with reason capture, persisted to review_finding_feedback and displayed as badges on finding cards. - Suppression memory (ADE-27 flagship): new review_suppressions table plus reviewSuppressionService that embeds suppressed titles via the existing embedding service and KNN-matches candidate findings (embedding cosine with Jaccard fallback). Scope tiers: repo / path (glob) / global. Pre-publish filter skips matched findings and tags them with scope + similarity. Active suppressions inspectable and removable in a new Learnings panel. - Tool-backed evidence (ADE-26 finishing): reviewToolEvidence promotes CI, test, and session-failure signals from the validation context into first-class tool_signal Evidence entries on findings, capped at three per finding and gated on file or title overlap. - Inline diff excerpts: reviewDiffContext builds a highlighted ±8 line window around the anchored line from the materialized patch; rendered in the new ReviewFindingCard with add/del tinting and focus-line border. - Finding class UX: intent_drift / incomplete_rollout / late_stage_regression chips with explanatory tooltips and distinctive tones. - Edge cases: severity filter chips, filtered-count toggle, cancel-run control for running/queued reviews, retry for failed runs, dedicated No-findings empty state. - Learnings tab: quality metrics (runs, noise rate, addressed, dismissed, published, by-class), active suppressions list with hit counts, recent feedback log. - Backend: review service exposes cancelRun, recordFeedback, listSuppressions, deleteSuppression, qualityReport; new IPC channels + preload bridge + browser mocks. - Tests: 13 new tests (diff-context, tool-evidence, suppression service) plus schema updates to keep existing review service + UI suites green; 46 review-related tests now pass. - Merge fix: narrow ptyService resume-guard to startupCommand paths so terminal create-with-provided-id keeps working; keep strict behavior for codex-style resume attempts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- executeRun: guard against cancellation before the first pass and re-check after publishRun; clear both activeRuns and cancelledRuns in the finally so stale entries can't leak into a rerun. - recordFeedback (suppress): if the user picks "path" scope but the finding has no file path and no explicit pattern, fall back to "repo" so we never insert a suppression that can never match. - reviewToolEvidence: unrelated failing CI checks and test runs were being attached to every finding; tightened the gate to require title/body token overlap, and scaled the token threshold so single-word summaries like "typecheck" still qualify. - reviewDiffContext: when the anchored line falls outside every hunk, show the first hunk unsliced instead of filtering against an empty window (previous behavior left just the hunk header on screen). - New test covering the anchor-outside-hunk path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Unit tests for reviewDiffContext, reviewSuppressionService, reviewToolEvidence (21 tests). - Regenerate DatabaseBootstrap.sql for new pr_convergence_state / auto_converge_runs kvDb tables (iOS parity). - Add window.ade.app.getInfo mock to TabNav.test.tsx to match new useEffect. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds a complete “review” feature: new per-project reviewService, DB schema for runs/findings/feedback/suppressions, IPC/preload/renderer bridges, review execution/context/target/evidence tooling, PR publication integration, UI pages/components, and comprehensive tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@copilot review but do not make fixes |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Here's my review of the PR changes. ReviewSummaryThis is a large, well-architected addition (~12,600 lines) introducing a multi-pass review engine, PR publication, suppression memory, tool-backed evidence, and inline diff UI. The structural design is solid — clean service boundaries, good use of dependency injection, and thorough unit test coverage. A few issues are worth flagging. Issues1.
|
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| if (hunkMatch) { | ||
| currentNewLine = Number(hunkMatch[1] ?? "0"); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
GitHub's position parameter documentation states: "The position in the diff continues to increase through lines of whitespace and additional hunks until the beginning of a new file." This means every @@ hunk header after the first one in a file counts as a position in the diff. However, the parser's continue at line 128 unconditionally skips the currentDiffPosition += 1 increment for all hunk headers, not just the first one.
For a file with N hunks, all positions in hunks 2..N are off by (hunk_index - 1). This causes publishReviewPublication to submit inline comments at wrong diff positions, which GitHub either silently attaches to the wrong line or rejects.
// reviewTargetMaterializer.ts
const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/);
if (hunkMatch) {
currentNewLine = Number(hunkMatch[1] ?? "0");
continue; // skips currentDiffPosition++ for ALL hunk headers
}The fix: increment currentDiffPosition for hunk headers that are not the first in a file (i.e., when currentDiffPosition > 0). The first hunk header is correctly skipped since position 1 begins at the line below it, but subsequent hunk headers are lines in the diff that contribute to the position counter.
| db.run("delete from review_run_publications where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); | ||
| db.run("delete from review_findings where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The lane deletion cleanup explicitly deletes review_run_publications, review_findings, review_run_artifacts, and review_runs to avoid relying on CASCADE (the comment at line 2529 explains CRR conversion can strip checked foreign keys). However, review_finding_feedback — which references review_findings(id) with ON DELETE CASCADE — is not explicitly deleted. Following the same reasoning documented in the code, orphaned review_finding_feedback rows will accumulate when CRR is active.
// laneService.ts — missing line before review_findings deletion:
db.run("delete from review_run_publications where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]);
db.run("delete from review_findings where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]);
// ↑ review_finding_feedback should be deleted BEFORE review_findingsAdd: db.run("delete from review_finding_feedback where finding_id in (select id from review_findings where run_id in (select id from review_runs where lane_id = ? and project_id = ?))", [laneId, projectId]); before the review_findings deletion.
| db.run("delete from review_run_publications where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); | |
| db.run("delete from review_findings where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); | |
| db.run("delete from review_run_publications where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); | |
| db.run("delete from review_finding_feedback where finding_id in (select id from review_findings where run_id in (select id from review_runs where lane_id = ? and project_id = ?))", [laneId, projectId]); | |
| db.run("delete from review_findings where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (7)
apps/desktop/src/shared/types/prs.ts (1)
1072-1077: Consider documentingpositionsemantics on the inline comment type.The
positionfield here maps to GitHub's diff-position offset (lines in the unified diff), not a source-file line number. SinceprService.submitReviewRequestalready enforcesNumber.isFinite(position) && position > 0, a short doc comment here would help renderer/IPC callers avoid conflatingpositionwith source line numbers when building inline review payloads (e.g., fromreviewDiffContext±8-line excerpts).✏️ Proposed doc addition
- comments?: Array<{ - path: string; - body: string; - position: number; - }>; + comments?: Array<{ + path: string; + body: string; + /** Offset within the unified diff (1-based), not a source-file line number. See GitHub's PR reviews API. */ + position: number; + }>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/types/prs.ts` around lines 1072 - 1077, The inline comment type's position field is ambiguous: document that comments?: Array<{ path, body, position }> uses GitHub's diff-position offset (unified-diff line index) rather than a source-file line number; add a short doc comment above the comments property describing the semantics and the invariant (position is a positive finite diff-position as enforced by prService.submitReviewRequest) and mention callers like reviewDiffContext to avoid conflation when building inline review payloads.apps/desktop/src/main/services/lanes/laneService.ts (1)
2527-2547: Consider wrapping lane-delete DB cleanup in a single transaction.The 16 sequential
db.run(...)deletes (now extended with 4 review-table deletes) are not atomic. If the process crashes between, say, thereview_run_publicationsdelete and the finallanesrow delete, the lane row persists while half of its dependent rows are gone — leaving the Learnings panel and publication history showing references to a defunct lane. Sincenode:sqlitesupportsBEGIN/COMMIT, wrapping this block is cheap and converts partial-failure into a clean retry.Not introduced by this PR, but the new review deletes widen the failure surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/lanes/laneService.ts` around lines 2527 - 2547, The series of db.run(...) deletion calls in the lane deletion path should be executed inside a single database transaction so the cleanup is atomic; wrap the block that runs db.run(...) (the deletes that reference laneId/projectId and the logSlowDeleteStep("database_cleanup", dbCleanupStartedAt) call) with BEGIN and COMMIT and ensure you issue ROLLBACK on any error, using the same sqlite client methods (db.run/exec) you already use, and propagate or handle errors so partial deletes cannot persist (ensure transaction uses laneId/projectId parameters unchanged and that the final deletion of lanes and the logSlowDeleteStep occur only after a successful COMMIT).apps/desktop/src/renderer/components/review/reviewRouteState.test.ts (1)
1-14: Consider adding a round-trip case for missingrunIdquery param.Coverage is solid for the whitespace and trimmed-id paths. One small gap:
readReviewRunId("")andreadReviewRunId("?other=1")aren't exercised, yetReviewPagesyncs state fromlocation.searchon navigation — confirming the "no runId" branch returns""/nullconsistently would lock in the contract both functions share.🧪 Proposed additional case
describe("reviewRouteState", () => { it("omits whitespace-only run ids from the search string", () => { expect(buildReviewSearch(" ")).toBe(""); }); it("round-trips a trimmed run id", () => { const search = buildReviewSearch(" run-123 "); expect(search).toBe("?runId=run-123"); expect(readReviewRunId(search)).toBe("run-123"); }); + + it("returns an empty run id when search has no runId param", () => { + expect(readReviewRunId("")).toBe(""); + expect(readReviewRunId("?other=1")).toBe(""); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/review/reviewRouteState.test.ts` around lines 1 - 14, Add a test that exercises the "no runId" branch by asserting readReviewRunId returns an empty string (or the established sentinel this codebase uses) for both an empty search and a search with unrelated params, and assert buildReviewSearch of an empty/undefined run id produces an empty string; specifically add assertions calling readReviewRunId("") and readReviewRunId("?other=1") expecting "" (matching the contract used by buildReviewSearch) and confirm buildReviewSearch("") (or buildReviewSearch(undefined) if applicable) === "" to ensure the round-trip behavior between buildReviewSearch and readReviewRunId is consistent.apps/desktop/src/main/services/processes/processService.ts (1)
588-596: Swallowed error hides actionable failure modes.The
try/catchcollapses everystatSync/realpathSyncfailure intocwd does not exist, butstatSynccan also fail withEACCES(no read/execute permission),ENOTDIR,ELOOP, etc. Surfacing the real cause makes process-start failures diagnosable from logs.♻️ Suggested refactor
if (allowExternalCwd) { - try { - const resolved = path.resolve(configuredCwd); - const stat = fs.statSync(resolved); - if (!stat.isDirectory()) throw new Error("Path is not a directory"); - cwd = fs.realpathSync(resolved); - } catch { - throw new Error(`Process '${definition.id}' cwd does not exist: ${configuredCwd}`); - } + const resolved = path.resolve(configuredCwd); + let stat: fs.Stats; + try { + stat = fs.statSync(resolved); + } catch (err) { + const code = (err as NodeJS.ErrnoException)?.code ?? "UNKNOWN"; + throw new Error(`Process '${definition.id}' cwd is not accessible (${code}): ${configuredCwd}`); + } + if (!stat.isDirectory()) { + throw new Error(`Process '${definition.id}' cwd is not a directory: ${configuredCwd}`); + } + cwd = fs.realpathSync(resolved); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/processes/processService.ts` around lines 588 - 596, The try/catch around resolving configuredCwd swallows the real filesystem error; modify the block in processService.ts (around allowExternalCwd handling and the path resolution using path.resolve, fs.statSync, fs.realpathSync) to capture the caught error (e.g., err) and include its message (or set it as the cause) in the thrown Error for Process '${definition.id}' so failures like EACCES/ENOTDIR/ELOOP surface; ensure you still validate isDirectory and preserve the original error information when throwing the new error referencing configuredCwd and definition.id.apps/desktop/src/main/services/review/reviewContextBuilder.test.ts (1)
1-125: Extract the shared reviewsql.jsharness before it drifts.This bootstrap/row-mapper/
AdeDbshim is now duplicated here and inapps/desktop/src/main/services/review/reviewSuppressionService.test.ts. The next schema or adapter tweak will leave one of these suites validating a different setup than the other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/review/reviewContextBuilder.test.ts` around lines 1 - 125, Duplicate sql.js test harness (mapExecRows, AdeDb type, createInMemoryAdeDb, and SQL init) should be extracted into a shared test utility module and imported by both reviewContextBuilder.test.ts and reviewSuppressionService.test.ts; create a new file (e.g., test/utils/sqlHarness.ts) that exports mapExecRows, AdeDb, createInMemoryAdeDb, and the SQL initialization helper or SQL instance, update both tests to import these symbols instead of defining them inline, and ensure exported helpers initialize SQL (initSqlJs) once and expose createInMemoryAdeDb, mapExecRows, and the AdeDb types so both suites use the exact same schema and adapter shim.apps/desktop/src/preload/preload.test.ts (1)
80-126: Cover the rest of the review bridge contract here.This only exercises part of
window.ade.review. The same surface now also exposescancelRun,recordFeedback,listSuppressions,deleteSuppression, andqualityReport, so a channel mismatch there would still pass. A small table-driven sweep over the full bridge would make this contract much harder to drift.As per coding guidelines
apps/desktop/**/{ipc,preload,types,shared}/**/*.{ts,tsx}: Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/preload/preload.test.ts` around lines 80 - 126, The test only covers part of window.ade.review; add a table-driven test that iterates the full bridge surface—methods cancelRun, recordFeedback, listSuppressions, deleteSuppression, qualityReport (in addition to the already-tested listLaunchContext, listRuns, getRunDetail, startRun, rerun)—and for each entry call bridge.review.<method>(...sampleArgs) and assert ipcRenderer.invoke was called with the corresponding IPC constant (e.g., IPC.reviewCancelRun, IPC.reviewRecordFeedback, IPC.reviewListSuppressions, IPC.reviewDeleteSuppression, IPC.reviewQualityReport) and the expected payload; keep using the same mocked exposeInMainWorld/ipcRenderer and also include checks that event subscription uses IPC.reviewEvent and removal still calls removeListener with the listener returned earlier. Ensure you reference bridge.review and the IPC constants (IPC.reviewCancelRun, IPC.reviewRecordFeedback, IPC.reviewListSuppressions, IPC.reviewDeleteSuppression, IPC.reviewQualityReport) so the test fails if any channel/method drifts.apps/desktop/src/preload/preload.ts (1)
921-932: Prefer static type imports for review feedback/suppression types.The rest of this file imports all shared types via the single top-level
import type { ... } from "../shared/types"block. These four inlineimport("../shared/types").Review*types (ReviewRecordFeedbackArgs,ReviewFeedbackRecord,ReviewListSuppressionsArgs,ReviewSuppression,ReviewQualityReport) break that consistency for no runtime benefit. Moving them to the top import block matches the convention used forReviewEventPayload,ReviewLaunchContext, etc. added just above.♻️ Suggested consolidation
ReviewEventPayload, + ReviewFeedbackRecord, ReviewLaunchContext, ReviewListRunsArgs, + ReviewListSuppressionsArgs, + ReviewQualityReport, + ReviewRecordFeedbackArgs, ReviewRun, ReviewRunDetail, ReviewStartRunArgs, + ReviewSuppression,- recordFeedback: async ( - args: import("../shared/types").ReviewRecordFeedbackArgs, - ): Promise<import("../shared/types").ReviewFeedbackRecord> => - ipcRenderer.invoke(IPC.reviewRecordFeedback, args), - listSuppressions: async ( - args: import("../shared/types").ReviewListSuppressionsArgs = {}, - ): Promise<import("../shared/types").ReviewSuppression[]> => - ipcRenderer.invoke(IPC.reviewListSuppressions, args), - deleteSuppression: async (suppressionId: string): Promise<boolean> => - ipcRenderer.invoke(IPC.reviewDeleteSuppression, { suppressionId }), - qualityReport: async (): Promise<import("../shared/types").ReviewQualityReport> => + recordFeedback: async (args: ReviewRecordFeedbackArgs): Promise<ReviewFeedbackRecord> => + ipcRenderer.invoke(IPC.reviewRecordFeedback, args), + listSuppressions: async ( + args: ReviewListSuppressionsArgs = {}, + ): Promise<ReviewSuppression[]> => + ipcRenderer.invoke(IPC.reviewListSuppressions, args), + deleteSuppression: async (suppressionId: string): Promise<boolean> => + ipcRenderer.invoke(IPC.reviewDeleteSuppression, { suppressionId }), + qualityReport: async (): Promise<ReviewQualityReport> => ipcRenderer.invoke(IPC.reviewQualityReport),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/preload/preload.ts` around lines 921 - 932, The inline type-imports used in the IPC wrapper methods recordFeedback, listSuppressions, deleteSuppression, and qualityReport should be moved to the file's top-level type import block to match the existing pattern (e.g., where ReviewEventPayload and ReviewLaunchContext are imported); update the top import type list to include ReviewRecordFeedbackArgs, ReviewFeedbackRecord, ReviewListSuppressionsArgs, ReviewSuppression, and ReviewQualityReport, then replace the inline import(...) type annotations in those function signatures with the corresponding named types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/main.ts`:
- Around line 3707-3711: The fast-shutdown path (runImmediateProcessCleanup)
currently skips disposing ctx.reviewService causing in-flight reviews to
continue; update the fast-shutdown/cleanup routine (runImmediateProcessCleanup)
to call and await ctx.reviewService?.dispose?.() (or invoke the same disposal
logic used in normal teardown) and wrap it in a try/catch similar to the normal
context teardown block to ensure it runs safely on
uncaughtException/SIGTERM/forced exit so the reviewService is properly stopped
before process exit.
In `@apps/desktop/src/main/services/lanes/laneLaunchContext.ts`:
- Around line 55-63: The current early-return for allowExternalCwd in
laneLaunchContext.ts allows any absolute requestedCwd to bypass
resolvePathWithinRoot and be used directly (propagated to ptyService.ts),
creating a security escape; change the branch so that when args.allowExternalCwd
is true you validate requestedCwd against an explicit trusted roots whitelist
(e.g., configured trustedRootPaths) and only accept it if
path.resolve(requestedCwd) is inside one of those trusted roots, otherwise fall
back to resolvePathWithinRoot(requestedCwd) or reject; keep the use of
ensureDirectoryExists for existence checks and preserve laneWorktreePath
behavior, updating references to allowExternalCwd, resolvePathWithinRoot, and
ensureDirectoryExists in laneLaunchContext.ts accordingly so callers like
ptyService.ts receive only vetted paths.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 2535-2538: The cleanup sequence in laneService.ts fails to
explicitly remove review_finding_feedback rows, risking orphaned feedback if FK
constraints were stripped; add a db.run deletion for review_finding_feedback
that targets feedback whose finding_id is in findings for runs of the given
lane/project (i.e., before the existing deletion of review_findings), using the
same parameter array [laneId, projectId] and matching the pattern used by the
other db.run calls that reference review_runs/review_findings.
In `@apps/desktop/src/main/services/processes/processService.ts`:
- Around line 585-608: The overlay cwd handling in processService currently lets
absolute configuredCwd (checked via allowExternalCwd) bypass containment
validation; update the block that handles allowExternalCwd to validate the
resolved absolute path against the lane/project root (or an explicit allowlist)
before accepting it—reuse or mirror the containment logic used by
resolvePathWithinRoot (or call a new helper like validatePathWithinRoot) and
throw the same ProcessDefinition-style errors when the path is outside or
missing; alternatively, if absolute overlay cwds are intentionally permitted,
add a clear docstring on ProcessDefinition.cwd and LaneOverlayOverrides.cwd
explaining that overlay absolute paths bypass containment checks.
In `@apps/desktop/src/main/services/prs/prService.ts`:
- Around line 278-290: buildPublishedReviewCommentBody currently inserts raw
finding.body and evidence.quote into PR comments; update this function (and the
code path that posts findings to GitHub) to run both finding.body and the
extracted quote through the project’s existing redaction/sanitization utility
before including them, and if no safe redacted output can be produced or the
evidence type is non-diff, suppress publishing that evidence (only include
severity/title/confidence). Preserve the code fence and length truncation logic
for quote but apply truncation after redaction, and ensure you reference
ReviewFinding when locating these fields to change.
- Around line 2106-2108: The follow-up refresh after creating the review must be
best-effort: wrap the call to refreshOne(args.prId) (and/or
markHotRefresh([args.prId])) inside a try/catch in submitReviewRequest so any
error from refreshOne is caught, logged, and not rethrown; ensure
publishReviewPublication is called based on the POST success (not the refresh
result) so a refresh failure does not mark the publication as failed or cause
duplicate retries. Use submitReviewRequest, refreshOne, markHotRefresh, and
publishReviewPublication to locate and update the flow.
In `@apps/desktop/src/main/services/review/reviewContextBuilder.ts`:
- Around line 523-533: The loop over normalizedChecks currently marks any
completed check whose conclusion is not "success" as a failure, which
incorrectly includes "neutral", "skipped", and "cancelled"; update the failure
detection in the for loop that populates signals (the normalizedChecks iteration
that uses VALIDATION_SIGNAL_LIMIT and pushes into signals via clipText) so that
a signal is only created when the check's conclusion is explicitly "failure"
(i.e., change the failed condition to require check.conclusion === "failure"
rather than completed && conclusion !== "success"), leaving the rest of the push
logic (summary, filePaths, sourceId) unchanged.
In `@apps/desktop/src/main/services/review/reviewDiffContext.ts`:
- Around line 72-80: The current filter unconditionally keeps deletions because
it only checks entry.line == null; instead, only retain deletion entries that
fall within the desired context window around the target lines. Update the loop
over parsed.lines so that when entry.line == null you check the deletion's
original line number (e.g., entry.oldLine or entry.originalLine) or its
proximity to the nearest non-deleted entry and only push entry into filtered if
that line number is within the ±contextWindow (e.g., ±8) of the current
hunk/target lines; otherwise skip the deletion. Ensure you reference
parsed.lines and the entry.line==null branch (and use
entry.oldLine/entry.originalLine or nearest non-null entry) when implementing
the conditional.
- Around line 113-115: The computed endLine can become less than startLine for
pure deletions (chosen.newLength === 0) which yields an inverted range; update
the logic around lineNumbers/startLine/endLine in ReviewDiffContext so that the
fallback guarantees a non-inverted range — compute startLine as you already do,
compute endLine as the max of the calculated value and startLine (e.g., use
Math.max(calculatedEnd, startLine)) or otherwise clamp using chosen.newStart and
chosen.newLength so endLine >= startLine; reference the variables sliced.lines,
lineNumbers, startLine, endLine, chosen.newStart and chosen.newLength when
making the fix.
In `@apps/desktop/src/main/services/review/reviewService.ts`:
- Around line 2078-2089: The code still calls await publishRun(...) even after
the cancellation gate, risking posting an external GitHub review after user
cancellation; update the flow to re-check cancellation before publishing or make
publishRun cancellation-aware. Specifically, either (A) check the run's
cancellation flag/state for runId (or a local isCancelled variable set by the
cancellation gate) and skip/return early instead of calling publishRun, or (B)
extend publishRun to accept an AbortSignal/timeout and pass the current
cancellation signal so publishRun can abort and not post the external review;
reference publishRun, runId, materialized, adjudication, and effectiveRun when
making the change. Ensure no external publish is attempted when cancellation is
detected.
- Around line 2323-2330: The code silently converts a requested path-level
suppression into a repo-level suppression when no usable file path is provided;
update the logic in the function around requestedScope/requestedPathPattern
(variables: requestedScope, requestedPathPattern, hasUsablePathPattern, scope,
pathPattern) so that if requestedScope === "path" and hasUsablePathPattern is
false you do not widen scope but instead return an error (or throw) indicating a
missing/invalid pathPattern to the caller; ensure callers receive a clear
failure rather than silently storing a repo-wide suppression.
In `@apps/desktop/src/main/services/review/reviewSuppressionService.ts`:
- Around line 287-289: The current check lets a class-scoped suppression match
an unclassified finding because it requires both suppression.findingClass and
args.finding.findingClass to be truthy; change the guard to only require the
suppression to have a class and then compare it to the finding: replace the
condition with something like "if (suppression.findingClass &&
suppression.findingClass !== args.finding.findingClass) continue;" so any
suppression with a findingClass will not match when args.finding.findingClass is
null/undefined.
In `@apps/desktop/src/main/services/review/reviewToolEvidence.ts`:
- Around line 105-106: The loop over args.validation.checks (for (const check of
args.validation.checks)) is allowing checks with null/undefined conclusions to
be treated like failures; change the guard so only checks whose conclusion is
explicitly "failure" or "action_required" are processed. Update the condition on
check.conclusion to explicitly skip when conclusion is missing or not one of
"failure" or "action_required" (e.g., add a null/undefined check or invert the
logic so you only proceed when check.conclusion === "failure" ||
check.conclusion === "action_required") to prevent queued/in-progress checks
from being attached as evidence.
In `@apps/desktop/src/preload/global.d.ts`:
- Line 1487: The new submitReview signature references SubmitPrReviewResult but
that type isn't imported into this declaration file; add an import (or include
in the existing type import list) for SubmitPrReviewResult so the submitReview:
(args: SubmitPrReviewArgs) => Promise<SubmitPrReviewResult> line typechecks.
Locate where other types like SubmitPrReviewArgs are imported and add
SubmitPrReviewResult to the same import declaration (or add a new type-only
import) to resolve the missing symbol referenced by submitReview.
In `@apps/desktop/src/renderer/components/review/reviewApi.ts`:
- Around line 15-27: ReviewBridge's types diverge from the preload contract;
change the signatures to match preload: set startRun: (args: ReviewStartRunArgs)
=> Promise<ReviewRun>, rerun: (runId: string) => Promise<ReviewRun>, and
listLaunchContext: () => Promise<ReviewLaunchContext> (non-nullable). Update
callers startReviewRun and rerunReview to expect a ReviewRun result and simplify
their normalization to return { runId: result?.id ?? null } accordingly, leaving
other optional bridge members unchanged.
In `@apps/desktop/src/renderer/components/review/ReviewFindingCard.tsx`:
- Around line 245-256: The modal currently allows closing while a submission is
in flight; update the close handlers so they no-op when submitting is true: in
the useEffect that adds the onKey handler (function onKey) check the submitting
flag and ignore Escape when submitting, and likewise guard the backdrop click
handler and the close button's onClick (the close button that uses
closeButtonRef) so they return early if submitting; also reflect disabled state
on the close button (e.g., aria-disabled / disabled) while submitting. Apply the
same guarding change to the second effect/handler block referenced around lines
295-316 so all close paths (Escape, backdrop click, and X button) are blocked
during handleSubmit's submitting state.
In `@apps/desktop/src/renderer/components/review/ReviewLearningsPanel.tsx`:
- Around line 99-102: The remove handler currently assumes
deleteReviewSuppression(suppressionId) always succeeds; change handleRemove to
check the return value and handle a false result as a failure: after awaiting
deleteReviewSuppression(suppressionId) verify the result is truthy, and if it is
false then surface an error (e.g., set error state or call the existing UI
notification/toast) and return early so refresh() is not called; only call
refresh() when deleteReviewSuppression returns a successful truthy value. Ensure
you update handleRemove's async flow and any UI feedback path so users see a
failure when the bridge returns false.
In `@apps/desktop/src/renderer/components/review/ReviewPage.tsx`:
- Around line 821-825: The validation currently allows launching a lane diff
when compareLaneId still equals the source lane (after the user switched lanes),
causing buildTargetConfig() to launch a lane-vs-itself review; update the
launchValidationMessage logic to also check that
launchDraft.compareLaneId.trim() !== launchDraft.laneId.trim() (and return an
error like "Choose another lane to compare against" or "Cannot compare a lane to
itself") when launchDraft.targetMode === "lane_diff" and launchDraft.compareKind
=== "lane", or alternatively clear launchDraft.compareLaneId when the source
lane (launchDraft.laneId) changes so buildTargetConfig() never receives a
same-vs-same pair.
- Around line 968-977: The path-root matching in resolveFindingTarget is too
permissive and can match sibling worktrees (e.g., "/repo" matching
"/repo-bugfix"); normalize and perform a boundary-aware check: derive rootPath
from lane.worktreePath, ensure both path and rootPath are normalized (trim
trailing slashes), then consider path under rootPath only if path === rootPath
or path.startsWith(rootPath + "/"); use that condition when computing target so
you only strip the true worktree prefix and avoid rewriting sibling paths.
In `@apps/ios/ADE/Resources/DatabaseBootstrap.sql`:
- Around line 2448-2452: The script contains duplicate ALTER TABLE statements
that re-add columns already declared in the CREATE TABLE for review_findings;
remove the three ALTER TABLE lines that add finding_class,
originating_passes_json, and adjudication_json (the statements containing "alter
table review_findings add column finding_class text;", "alter table
review_findings add column originating_passes_json text;", and "alter table
review_findings add column adjudication_json text;") so the bootstrap won't fail
with "duplicate column name" on fresh installs; alternatively, if this file must
be idempotent, replace those statements with a runtime guard that checks PRAGMA
table_info('review_findings') for each column before adding.
---
Nitpick comments:
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 2527-2547: The series of db.run(...) deletion calls in the lane
deletion path should be executed inside a single database transaction so the
cleanup is atomic; wrap the block that runs db.run(...) (the deletes that
reference laneId/projectId and the logSlowDeleteStep("database_cleanup",
dbCleanupStartedAt) call) with BEGIN and COMMIT and ensure you issue ROLLBACK on
any error, using the same sqlite client methods (db.run/exec) you already use,
and propagate or handle errors so partial deletes cannot persist (ensure
transaction uses laneId/projectId parameters unchanged and that the final
deletion of lanes and the logSlowDeleteStep occur only after a successful
COMMIT).
In `@apps/desktop/src/main/services/processes/processService.ts`:
- Around line 588-596: The try/catch around resolving configuredCwd swallows the
real filesystem error; modify the block in processService.ts (around
allowExternalCwd handling and the path resolution using path.resolve,
fs.statSync, fs.realpathSync) to capture the caught error (e.g., err) and
include its message (or set it as the cause) in the thrown Error for Process
'${definition.id}' so failures like EACCES/ENOTDIR/ELOOP surface; ensure you
still validate isDirectory and preserve the original error information when
throwing the new error referencing configuredCwd and definition.id.
In `@apps/desktop/src/main/services/review/reviewContextBuilder.test.ts`:
- Around line 1-125: Duplicate sql.js test harness (mapExecRows, AdeDb type,
createInMemoryAdeDb, and SQL init) should be extracted into a shared test
utility module and imported by both reviewContextBuilder.test.ts and
reviewSuppressionService.test.ts; create a new file (e.g.,
test/utils/sqlHarness.ts) that exports mapExecRows, AdeDb, createInMemoryAdeDb,
and the SQL initialization helper or SQL instance, update both tests to import
these symbols instead of defining them inline, and ensure exported helpers
initialize SQL (initSqlJs) once and expose createInMemoryAdeDb, mapExecRows, and
the AdeDb types so both suites use the exact same schema and adapter shim.
In `@apps/desktop/src/preload/preload.test.ts`:
- Around line 80-126: The test only covers part of window.ade.review; add a
table-driven test that iterates the full bridge surface—methods cancelRun,
recordFeedback, listSuppressions, deleteSuppression, qualityReport (in addition
to the already-tested listLaunchContext, listRuns, getRunDetail, startRun,
rerun)—and for each entry call bridge.review.<method>(...sampleArgs) and assert
ipcRenderer.invoke was called with the corresponding IPC constant (e.g.,
IPC.reviewCancelRun, IPC.reviewRecordFeedback, IPC.reviewListSuppressions,
IPC.reviewDeleteSuppression, IPC.reviewQualityReport) and the expected payload;
keep using the same mocked exposeInMainWorld/ipcRenderer and also include checks
that event subscription uses IPC.reviewEvent and removal still calls
removeListener with the listener returned earlier. Ensure you reference
bridge.review and the IPC constants (IPC.reviewCancelRun,
IPC.reviewRecordFeedback, IPC.reviewListSuppressions,
IPC.reviewDeleteSuppression, IPC.reviewQualityReport) so the test fails if any
channel/method drifts.
In `@apps/desktop/src/preload/preload.ts`:
- Around line 921-932: The inline type-imports used in the IPC wrapper methods
recordFeedback, listSuppressions, deleteSuppression, and qualityReport should be
moved to the file's top-level type import block to match the existing pattern
(e.g., where ReviewEventPayload and ReviewLaunchContext are imported); update
the top import type list to include ReviewRecordFeedbackArgs,
ReviewFeedbackRecord, ReviewListSuppressionsArgs, ReviewSuppression, and
ReviewQualityReport, then replace the inline import(...) type annotations in
those function signatures with the corresponding named types.
In `@apps/desktop/src/renderer/components/review/reviewRouteState.test.ts`:
- Around line 1-14: Add a test that exercises the "no runId" branch by asserting
readReviewRunId returns an empty string (or the established sentinel this
codebase uses) for both an empty search and a search with unrelated params, and
assert buildReviewSearch of an empty/undefined run id produces an empty string;
specifically add assertions calling readReviewRunId("") and
readReviewRunId("?other=1") expecting "" (matching the contract used by
buildReviewSearch) and confirm buildReviewSearch("") (or
buildReviewSearch(undefined) if applicable) === "" to ensure the round-trip
behavior between buildReviewSearch and readReviewRunId is consistent.
In `@apps/desktop/src/shared/types/prs.ts`:
- Around line 1072-1077: The inline comment type's position field is ambiguous:
document that comments?: Array<{ path, body, position }> uses GitHub's
diff-position offset (unified-diff line index) rather than a source-file line
number; add a short doc comment above the comments property describing the
semantics and the invariant (position is a positive finite diff-position as
enforced by prService.submitReviewRequest) and mention callers like
reviewDiffContext to avoid conflation when building inline review payloads.
🪄 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
Run ID: 0ecdc47f-8eef-4581-bbf7-c53e8c23bc04
📒 Files selected for processing (48)
apps/desktop/src/main/main.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneLaunchContext.test.tsapps/desktop/src/main/services/lanes/laneLaunchContext.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/processes/processService.test.tsapps/desktop/src/main/services/processes/processService.tsapps/desktop/src/main/services/prs/prService.reviewPublication.test.tsapps/desktop/src/main/services/prs/prService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/review/reviewContextBuilder.test.tsapps/desktop/src/main/services/review/reviewContextBuilder.tsapps/desktop/src/main/services/review/reviewDiffContext.test.tsapps/desktop/src/main/services/review/reviewDiffContext.tsapps/desktop/src/main/services/review/reviewRuleRegistry.tsapps/desktop/src/main/services/review/reviewService.test.tsapps/desktop/src/main/services/review/reviewService.tsapps/desktop/src/main/services/review/reviewSuppressionService.test.tsapps/desktop/src/main/services/review/reviewSuppressionService.tsapps/desktop/src/main/services/review/reviewTargetMaterializer.test.tsapps/desktop/src/main/services/review/reviewTargetMaterializer.tsapps/desktop/src/main/services/review/reviewToolEvidence.test.tsapps/desktop/src/main/services/review/reviewToolEvidence.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/app/TabNav.test.tsxapps/desktop/src/renderer/components/app/TabNav.tsxapps/desktop/src/renderer/components/review/ReviewFindingCard.tsxapps/desktop/src/renderer/components/review/ReviewLearningsPanel.tsxapps/desktop/src/renderer/components/review/ReviewPage.test.tsxapps/desktop/src/renderer/components/review/ReviewPage.tsxapps/desktop/src/renderer/components/review/reviewApi.tsapps/desktop/src/renderer/components/review/reviewRouteState.test.tsapps/desktop/src/renderer/components/review/reviewRouteState.tsapps/desktop/src/renderer/components/review/reviewTypes.tsapps/desktop/src/renderer/components/run/AddCommandDialog.tsxapps/desktop/src/renderer/components/shared/ReviewLaunchModelControls.tsxapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/index.tsapps/desktop/src/shared/types/prs.tsapps/desktop/src/shared/types/review.tsapps/desktop/src/shared/types/sessions.tsapps/ios/ADE/Resources/DatabaseBootstrap.sql
| try { | ||
| ctx.reviewService?.dispose?.(); | ||
| } catch { | ||
| // ignore | ||
| } |
There was a problem hiding this comment.
Dispose reviewService in the fast-shutdown path too.
Lines 3707-3709 cover normal context teardown, but Line 4233's runImmediateProcessCleanup() still skips the new ctx.reviewService. On uncaughtException, SIGTERM, or forced exit, an in-flight review can keep running/publishing while the rest of the project context is already being torn down.
Suggested follow-up
for (const ctx of contexts) {
try {
+ ctx.reviewService?.dispose?.();
+ } catch {
+ // ignore
+ }
+ try {
ctx.aiOrchestratorService?.dispose?.();
} catch {
// ignore
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/main.ts` around lines 3707 - 3711, The fast-shutdown
path (runImmediateProcessCleanup) currently skips disposing ctx.reviewService
causing in-flight reviews to continue; update the fast-shutdown/cleanup routine
(runImmediateProcessCleanup) to call and await ctx.reviewService?.dispose?.()
(or invoke the same disposal logic used in normal teardown) and wrap it in a
try/catch similar to the normal context teardown block to ensure it runs safely
on uncaughtException/SIGTERM/forced exit so the reviewService is properly
stopped before process exit.
| if (args.allowExternalCwd === true && path.isAbsolute(requestedCwd)) { | ||
| return { | ||
| laneWorktreePath: laneRoot, | ||
| cwd: ensureDirectoryExists( | ||
| path.resolve(requestedCwd), | ||
| `Requested cwd '${requestedCwd}' is not an existing directory.`, | ||
| ), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Reintroduce a boundary for external CWDs.
This branch now skips resolvePathWithinRoot() for any absolute path and only checks that the directory exists. apps/desktop/src/main/services/pty/ptyService.ts:1001-1030 then uses the returned cwd directly, so any caller that can set allowExternalCwd can launch work anywhere on the host filesystem, not just inside the selected lane or project. If this escape hatch is required, constrain it to an explicit trusted root set instead of treating every absolute path as valid.
As per coding guidelines apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/lanes/laneLaunchContext.ts` around lines 55 -
63, The current early-return for allowExternalCwd in laneLaunchContext.ts allows
any absolute requestedCwd to bypass resolvePathWithinRoot and be used directly
(propagated to ptyService.ts), creating a security escape; change the branch so
that when args.allowExternalCwd is true you validate requestedCwd against an
explicit trusted roots whitelist (e.g., configured trustedRootPaths) and only
accept it if path.resolve(requestedCwd) is inside one of those trusted roots,
otherwise fall back to resolvePathWithinRoot(requestedCwd) or reject; keep the
use of ensureDirectoryExists for existence checks and preserve laneWorktreePath
behavior, updating references to allowExternalCwd, resolvePathWithinRoot, and
ensureDirectoryExists in laneLaunchContext.ts accordingly so callers like
ptyService.ts receive only vetted paths.
| function buildPublishedReviewCommentBody(finding: ReviewFinding): string { | ||
| const sections = [ | ||
| `[${finding.severity.toUpperCase()}] ${finding.title}`, | ||
| "", | ||
| finding.body, | ||
| "", | ||
| `Confidence: ${Math.round(finding.confidence * 100)}%`, | ||
| ]; | ||
| const quote = finding.evidence.find((entry) => typeof entry.quote === "string" && entry.quote.trim().length > 0)?.quote?.trim() ?? null; | ||
| if (quote) { | ||
| sections.push("", "```", quote.slice(0, 1_200), "```"); | ||
| } | ||
| return sections.join("\n"); |
There was a problem hiding this comment.
Redact review content before publishing it to GitHub.
Lines 282-289 and Lines 323-325 post raw finding.body / evidence.quote into PR comments. With the new tool-backed evidence flow, that can leak secrets, tokens, or PII from CI/test/session output into the GitHub review. Please run publishable text through the same redaction/sanitization path used for logs/artifacts, or suppress publication for findings backed by non-diff evidence.
Also applies to: 323-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/prs/prService.ts` around lines 278 - 290,
buildPublishedReviewCommentBody currently inserts raw finding.body and
evidence.quote into PR comments; update this function (and the code path that
posts findings to GitHub) to run both finding.body and the extracted quote
through the project’s existing redaction/sanitization utility before including
them, and if no safe redacted output can be produced or the evidence type is
non-diff, suppress publishing that evidence (only include
severity/title/confidence). Preserve the code fence and length truncation logic
for quote but apply truncation after redaction, and ensure you reference
ReviewFinding when locating these fields to change.
| React.useEffect(() => { | ||
| if (!open) return; | ||
| const onKey = (event: KeyboardEvent) => { | ||
| if (event.key === "Escape") { | ||
| event.stopPropagation(); | ||
| onClose(); | ||
| } | ||
| }; | ||
| window.addEventListener("keydown", onKey); | ||
| // Move focus into the dialog so keyboard users are oriented. | ||
| queueMicrotask(() => closeButtonRef.current?.focus()); | ||
| return () => window.removeEventListener("keydown", onKey); |
There was a problem hiding this comment.
Don’t let the dialog close while feedback is being saved.
Once handleSubmit() flips submitting to true, Escape, backdrop clicks, and the X button still close the modal. That makes it easy to reopen the dialog and create duplicate feedback/suppression records while the first request is still in flight.
🛠️ Proposed fix
function DismissModal({ open, initialKind, finding, onClose, onSubmit }: DismissModalProps) {
@@
const [submitting, setSubmitting] = React.useState(false);
const closeButtonRef = React.useRef<HTMLButtonElement | null>(null);
+ const requestClose = React.useCallback(() => {
+ if (!submitting) onClose();
+ }, [submitting, onClose]);
@@
React.useEffect(() => {
if (!open) return;
const onKey = (event: KeyboardEvent) => {
- if (event.key === "Escape") {
+ if (event.key === "Escape" && !submitting) {
event.stopPropagation();
onClose();
}
};
@@
- }, [open, onClose]);
+ }, [open, onClose, submitting]);
@@
- onClick={onClose}
+ onClick={requestClose}
@@
- onClick={onClose}
+ onClick={requestClose}Also applies to: 295-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/review/ReviewFindingCard.tsx` around
lines 245 - 256, The modal currently allows closing while a submission is in
flight; update the close handlers so they no-op when submitting is true: in the
useEffect that adds the onKey handler (function onKey) check the submitting flag
and ignore Escape when submitting, and likewise guard the backdrop click handler
and the close button's onClick (the close button that uses closeButtonRef) so
they return early if submitting; also reflect disabled state on the close button
(e.g., aria-disabled / disabled) while submitting. Apply the same guarding
change to the second effect/handler block referenced around lines 295-316 so all
close paths (Escape, backdrop click, and X button) are blocked during
handleSubmit's submitting state.
| const handleRemove = React.useCallback(async (suppressionId: string) => { | ||
| try { | ||
| await deleteReviewSuppression(suppressionId); | ||
| await refresh(); |
There was a problem hiding this comment.
Handle deleteReviewSuppression() returning false.
The bridge wrapper uses false as a failure/no-bridge result. This path currently refreshes silently, so the remove button can appear to work while nothing was actually deleted.
Suggested fix
const handleRemove = React.useCallback(async (suppressionId: string) => {
try {
- await deleteReviewSuppression(suppressionId);
+ const removed = await deleteReviewSuppression(suppressionId);
+ if (!removed) {
+ setError("Failed to remove suppression");
+ return;
+ }
await refresh();
} catch (err) {
setError(err instanceof Error ? err.message : "Failed to remove suppression");
}
}, [refresh]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleRemove = React.useCallback(async (suppressionId: string) => { | |
| try { | |
| await deleteReviewSuppression(suppressionId); | |
| await refresh(); | |
| const handleRemove = React.useCallback(async (suppressionId: string) => { | |
| try { | |
| const removed = await deleteReviewSuppression(suppressionId); | |
| if (!removed) { | |
| setError("Failed to remove suppression"); | |
| return; | |
| } | |
| await refresh(); | |
| } catch (err) { | |
| setError(err instanceof Error ? err.message : "Failed to remove suppression"); | |
| } | |
| }, [refresh]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/review/ReviewLearningsPanel.tsx` around
lines 99 - 102, The remove handler currently assumes
deleteReviewSuppression(suppressionId) always succeeds; change handleRemove to
check the return value and handle a false result as a failure: after awaiting
deleteReviewSuppression(suppressionId) verify the result is truthy, and if it is
false then surface an error (e.g., set error state or call the existing UI
notification/toast) and return early so refresh() is not called; only call
refresh() when deleteReviewSuppression returns a successful truthy value. Ensure
you update handleRemove's async flow and any UI feedback path so users see a
failure when the bridge returns false.
| const launchValidationMessage = React.useMemo(() => { | ||
| if (!launchDraft.laneId.trim()) return "Choose a lane before launching a review."; | ||
| if (launchDraft.targetMode === "lane_diff" && launchDraft.compareKind === "lane" && !launchDraft.compareLaneId.trim()) { | ||
| return "Choose another lane to compare against."; | ||
| } |
There was a problem hiding this comment.
Prevent launching a lane diff against the same lane.
If the user picks lane A, selects lane B as the comparison, and then switches the source lane to B, compareLaneId stays populated and this validation still passes. buildTargetConfig() will then launch a lane-vs-itself review.
Suggested fix
const launchValidationMessage = React.useMemo(() => {
if (!launchDraft.laneId.trim()) return "Choose a lane before launching a review.";
- if (launchDraft.targetMode === "lane_diff" && launchDraft.compareKind === "lane" && !launchDraft.compareLaneId.trim()) {
- return "Choose another lane to compare against.";
+ if (launchDraft.targetMode === "lane_diff" && launchDraft.compareKind === "lane") {
+ if (!launchDraft.compareLaneId.trim()) {
+ return "Choose another lane to compare against.";
+ }
+ if (launchDraft.compareLaneId === launchDraft.laneId) {
+ return "Choose a different lane to compare against.";
+ }
}
return commitRangeValidationMessage;
}, [commitRangeValidationMessage, launchDraft]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const launchValidationMessage = React.useMemo(() => { | |
| if (!launchDraft.laneId.trim()) return "Choose a lane before launching a review."; | |
| if (launchDraft.targetMode === "lane_diff" && launchDraft.compareKind === "lane" && !launchDraft.compareLaneId.trim()) { | |
| return "Choose another lane to compare against."; | |
| } | |
| const launchValidationMessage = React.useMemo(() => { | |
| if (!launchDraft.laneId.trim()) return "Choose a lane before launching a review."; | |
| if (launchDraft.targetMode === "lane_diff" && launchDraft.compareKind === "lane") { | |
| if (!launchDraft.compareLaneId.trim()) { | |
| return "Choose another lane to compare against."; | |
| } | |
| if (launchDraft.compareLaneId === launchDraft.laneId) { | |
| return "Choose a different lane to compare against."; | |
| } | |
| } | |
| return commitRangeValidationMessage; | |
| }, [commitRangeValidationMessage, launchDraft]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/review/ReviewPage.tsx` around lines 821
- 825, The validation currently allows launching a lane diff when compareLaneId
still equals the source lane (after the user switched lanes), causing
buildTargetConfig() to launch a lane-vs-itself review; update the
launchValidationMessage logic to also check that
launchDraft.compareLaneId.trim() !== launchDraft.laneId.trim() (and return an
error like "Choose another lane to compare against" or "Cannot compare a lane to
itself") when launchDraft.targetMode === "lane_diff" and launchDraft.compareKind
=== "lane", or alternatively clear launchDraft.compareLaneId when the source
lane (launchDraft.laneId) changes so buildTargetConfig() never receives a
same-vs-same pair.
| const resolveFindingTarget = React.useCallback((finding: ReviewFinding): { laneId: string; target: string; rootPath: string | null } | null => { | ||
| const path = finding.filePath?.trim(); | ||
| const laneId = selectedRun?.laneId ?? launchDraft.laneId; | ||
| if (!path || !laneId) return null; | ||
| const lane = laneById.get(laneId) ?? null; | ||
| const rootPath = lane?.worktreePath ?? null; | ||
| const target = rootPath && path.startsWith(rootPath) | ||
| ? path.slice(rootPath.length).replace(/^\/+/, "") | ||
| : path.startsWith("/") ? path.replace(/^\//, "") : path; | ||
| return { laneId, target, rootPath }; |
There was a problem hiding this comment.
Use a path-boundary check before trimming rootPath.
path.startsWith(rootPath) is too loose here. A lane rooted at /repo will also match /repo-bugfix/..., so findings from sibling worktrees can be rewritten to the wrong relative path and open the wrong file.
Suggested fix
const resolveFindingTarget = React.useCallback((finding: ReviewFinding): { laneId: string; target: string; rootPath: string | null } | null => {
const path = finding.filePath?.trim();
const laneId = selectedRun?.laneId ?? launchDraft.laneId;
if (!path || !laneId) return null;
const lane = laneById.get(laneId) ?? null;
const rootPath = lane?.worktreePath ?? null;
- const target = rootPath && path.startsWith(rootPath)
- ? path.slice(rootPath.length).replace(/^\/+/, "")
+ const normalizedRoot = rootPath?.replace(/[\\/]+$/, "") ?? null;
+ const withinRoot = normalizedRoot != null
+ && (path === normalizedRoot || path.startsWith(`${normalizedRoot}/`) || path.startsWith(`${normalizedRoot}\\`));
+ const target = withinRoot
+ ? path.slice(normalizedRoot.length).replace(/^[\\/]+/, "")
: path.startsWith("/") ? path.replace(/^\//, "") : path;
return { laneId, target, rootPath };
}, [laneById, launchDraft.laneId, selectedRun?.laneId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/review/ReviewPage.tsx` around lines 968
- 977, The path-root matching in resolveFindingTarget is too permissive and can
match sibling worktrees (e.g., "/repo" matching "/repo-bugfix"); normalize and
perform a boundary-aware check: derive rootPath from lane.worktreePath, ensure
both path and rootPath are normalized (trim trailing slashes), then consider
path under rootPath only if path === rootPath or path.startsWith(rootPath +
"/"); use that condition when computing target so you only strip the true
worktree prefix and avoid rewriting sibling paths.
| alter table review_findings add column finding_class text; | ||
|
|
||
| alter table review_findings add column originating_passes_json text; | ||
|
|
||
| alter table review_findings add column adjudication_json text; |
There was a problem hiding this comment.
Remove the duplicate ALTER TABLE statements for existing columns.
review_findings already defines finding_class, originating_passes_json, and adjudication_json in the CREATE TABLE above. On a fresh bootstrap, these ALTER TABLE statements will fail with duplicate column name and stop the script before the rest of the review schema is created.
Suggested fix
-alter table review_findings add column finding_class text;
-
-alter table review_findings add column originating_passes_json text;
-
-alter table review_findings add column adjudication_json text;
-
alter table review_findings add column diff_context_json text;
alter table review_findings add column suppression_match_json text;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ios/ADE/Resources/DatabaseBootstrap.sql` around lines 2448 - 2452, The
script contains duplicate ALTER TABLE statements that re-add columns already
declared in the CREATE TABLE for review_findings; remove the three ALTER TABLE
lines that add finding_class, originating_passes_json, and adjudication_json
(the statements containing "alter table review_findings add column finding_class
text;", "alter table review_findings add column originating_passes_json text;",
and "alter table review_findings add column adjudication_json text;") so the
bootstrap won't fail with "duplicate column name" on fresh installs;
alternatively, if this file must be idempotent, replace those statements with a
runtime guard that checks PRAGMA table_info('review_findings') for each column
before adding.
- reviewTargetMaterializer: increment diff position on non-first @@ hunks so GitHub inline-comment positions are correct across multi-hunk files (capy-ai). - laneService: explicitly delete review_finding_feedback before review_findings on lane deletion (CRR strips FKs, no cascade) (capy-ai). - reviewService.mapPublicationRow: drop tautological COMMENT?COMMENT:COMMENT ternary in favor of a direct cast (greptile). - reviewSuppressionService: cap PATH_GLOB_CACHE at 256 entries with FIFO eviction (greptile). - reviewService: build changedFilesByPath from the budget-sliced changedFiles, not materialized.changedFiles (greptile). Addresses PR #191 comments: 3136781788, 3136781797, 3136758435, 3136758509, 3136758600. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Since the last review, commit What was fixed in
|
- reviewService.byClass: wrap the latest-feedback subquery in a latest_only CTE (rn = 1 filter), so count(*) per class stops inflating when a finding has multiple feedback rows (greptile P1 #3136916987). - reviewToolEvidence.titleMatchesSignal: escape regex metachars in the interpolated token before `\b...\b` matching. Defensive; current tokenization strips these already (Copilot re-review item 1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Greptile P1 #3137014594 flags the absolute-cwd path as a sandbox bypass. Configured cwd originates from projectConfigService (same file that declares the command itself); there is no IPC path that lets UI input reach this code. Inline comment documents the reasoning so future reviewers don't relitigate the same question. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
Greptile P1 #3137082789 claims the first @@ header should count toward position. GitHub's REST docs are explicit: "the line just below the '@@' line is position 1", i.e. the first content line after the first hunk header is position 1, not the header itself. Inline comment documents this so future re-reviews don't re-flag the conditional gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot review but do not make fixes |
There was a problem hiding this comment.
♻️ Duplicate comments (4)
apps/desktop/src/main/services/review/reviewService.ts (2)
2076-2089:⚠️ Potential issue | 🔴 CriticalCancellation still allows external PR review to publish.
The gate at Line 2046 passes before
await publishRun(...).publishRunhas no cancel awareness, so a cancellation issued during that await posts the GitHub review anyway, while Lines 2093–2106 only flip local status tocancelled. Either re-checkcancelledRuns.has(runId)immediately before invokingprService.publishReviewPublicationinsidepublishRun, or thread anAbortSignalinto it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/review/reviewService.ts` around lines 2076 - 2089, The publishRun call can still post an external PR review after a cancellation because publishRun/prService.publishReviewPublication has no cancellation check; add cancellation awareness by re-checking cancelledRuns.has(runId) immediately before calling prService.publishReviewPublication inside publishRun (or accept and propagate an AbortSignal into publishRun and into prService.publishReviewPublication) and short-circuit publishing when cancelled so no external review is posted after cancellation.
2322-2330:⚠️ Potential issue | 🟠 MajorPath-scoped suppression silently widens to repo scope.
When the caller requests
scope: "path"but the finding has nofile_path(and no explicitpathPattern), this stores a repo-wide suppression instead of rejecting the request. A one-file mute silently becomes a repo-wide one. Prefer to throw or return an error so the caller can recover.🛠️ Proposed fix
const requestedScope: ReviewSuppressionScope = args.suppression?.scope ?? "repo"; const requestedPathPattern = args.suppression?.pathPattern ?? (requestedScope === "path" ? findingRow.file_path : null); const hasUsablePathPattern = typeof requestedPathPattern === "string" && requestedPathPattern.trim().length > 0; - const scope: ReviewSuppressionScope = requestedScope === "path" && !hasUsablePathPattern - ? "repo" - : requestedScope; + if (requestedScope === "path" && !hasUsablePathPattern) { + throw new Error("Path-scoped suppressions require a finding with a file path."); + } + const scope: ReviewSuppressionScope = requestedScope; const pathPattern = scope === "path" ? requestedPathPattern : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/review/reviewService.ts` around lines 2322 - 2330, The current suppression branch (when args.kind === "suppress") widens a requested path-scoped suppression to repo scope when no usable path pattern exists; update the validation so that if requestedScope === "path" and there is no usable path pattern (requestedPathPattern is null/empty and findingRow.file_path is falsy) the method returns/throws an error instead of switching scope to "repo". Locate the logic around requestedScope, requestedPathPattern, hasUsablePathPattern and scope in the args.kind === "suppress" block and replace the fallback that sets scope to "repo" with an explicit error/validation failure (include a clear message indicating path scope requires a pathPattern or findingRow.file_path) so callers can handle the failure rather than silently widening the suppression.apps/desktop/src/main/services/review/reviewToolEvidence.ts (1)
109-110:⚠️ Potential issue | 🟠 MajorQueued/in-progress checks still pass the guard.
The
check.conclusion &&short-circuit letsnull/undefinedconclusions (queued or in-progress) fall through and be attached as tool evidence. The check should also requirecheck.status === "completed"before evaluatingconclusion.🛠️ Proposed fix
for (const check of args.validation.checks) { - if (check.conclusion && check.conclusion !== "failure" && check.conclusion !== "action_required") continue; + if (check.status !== "completed") continue; + if (check.conclusion !== "failure" && check.conclusion !== "action_required") continue; const kind = classifyCheck(check.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/review/reviewToolEvidence.ts` around lines 109 - 110, The loop over args.validation.checks is allowing queued/in-progress checks because it only tests check.conclusion; update the guard inside the for (const check of args.validation.checks) to also require check.status === "completed" before evaluating conclusion (i.e., only proceed when check.status === "completed" and then ensure check.conclusion is not "failure" or "action_required"); reference the loop variable check and its properties conclusion and status in reviewToolEvidence.ts to locate and change the condition so non-completed checks are skipped.apps/desktop/src/main/services/review/reviewSuppressionService.ts (1)
292-294:⚠️ Potential issue | 🟠 MajorClass-scoped suppression still matches unclassified findings.
The guard requires both sides to have a
findingClassbefore it will skip on mismatch. When the new finding'sfindingClassisnull, a suppression created with a specific class (e.g.,late_stage_regression) silently matches, effectively broadening a class-specific suppression. Drop the&& args.finding.findingClassso any suppression with a class only matches findings of that same class.🛠️ Proposed fix
- if (suppression.findingClass && args.finding.findingClass && suppression.findingClass !== args.finding.findingClass) { + if (suppression.findingClass && suppression.findingClass !== args.finding.findingClass) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/review/reviewSuppressionService.ts` around lines 292 - 294, The guard in reviewSuppressionService.ts should not require args.finding.findingClass to be truthy; update the conditional that currently checks "suppression.findingClass && args.finding.findingClass && suppression.findingClass !== args.finding.findingClass" to only check suppression.findingClass and mismatch against args.finding.findingClass (i.e., change to "suppression.findingClass && suppression.findingClass !== args.finding.findingClass") so suppressions with a specific findingClass only match findings with the same class; modify the conditional where suppression.findingClass and args.finding.findingClass are referenced to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/main/services/review/reviewService.ts`:
- Around line 2076-2089: The publishRun call can still post an external PR
review after a cancellation because
publishRun/prService.publishReviewPublication has no cancellation check; add
cancellation awareness by re-checking cancelledRuns.has(runId) immediately
before calling prService.publishReviewPublication inside publishRun (or accept
and propagate an AbortSignal into publishRun and into
prService.publishReviewPublication) and short-circuit publishing when cancelled
so no external review is posted after cancellation.
- Around line 2322-2330: The current suppression branch (when args.kind ===
"suppress") widens a requested path-scoped suppression to repo scope when no
usable path pattern exists; update the validation so that if requestedScope ===
"path" and there is no usable path pattern (requestedPathPattern is null/empty
and findingRow.file_path is falsy) the method returns/throws an error instead of
switching scope to "repo". Locate the logic around requestedScope,
requestedPathPattern, hasUsablePathPattern and scope in the args.kind ===
"suppress" block and replace the fallback that sets scope to "repo" with an
explicit error/validation failure (include a clear message indicating path scope
requires a pathPattern or findingRow.file_path) so callers can handle the
failure rather than silently widening the suppression.
In `@apps/desktop/src/main/services/review/reviewSuppressionService.ts`:
- Around line 292-294: The guard in reviewSuppressionService.ts should not
require args.finding.findingClass to be truthy; update the conditional that
currently checks "suppression.findingClass && args.finding.findingClass &&
suppression.findingClass !== args.finding.findingClass" to only check
suppression.findingClass and mismatch against args.finding.findingClass (i.e.,
change to "suppression.findingClass && suppression.findingClass !==
args.finding.findingClass") so suppressions with a specific findingClass only
match findings with the same class; modify the conditional where
suppression.findingClass and args.finding.findingClass are referenced to
implement this change.
In `@apps/desktop/src/main/services/review/reviewToolEvidence.ts`:
- Around line 109-110: The loop over args.validation.checks is allowing
queued/in-progress checks because it only tests check.conclusion; update the
guard inside the for (const check of args.validation.checks) to also require
check.status === "completed" before evaluating conclusion (i.e., only proceed
when check.status === "completed" and then ensure check.conclusion is not
"failure" or "action_required"); reference the loop variable check and its
properties conclusion and status in reviewToolEvidence.ts to locate and change
the condition so non-completed checks are skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a91555c-ca1e-4b85-8c95-26f90d0b6fad
📒 Files selected for processing (6)
apps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/processes/processService.tsapps/desktop/src/main/services/review/reviewService.tsapps/desktop/src/main/services/review/reviewSuppressionService.tsapps/desktop/src/main/services/review/reviewTargetMaterializer.tsapps/desktop/src/main/services/review/reviewToolEvidence.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/services/processes/processService.ts
test-desktop (5) failed on CI: TerminalView's 'fits to container' test
expected renderer=webgl but got 'dom'. The test used fake timers while
the production path awaits a dynamic `import('@xterm/addon-webgl')` —
that microtask chain doesn't reliably settle under fake timers on CI
shards. Switch this one callsite to real timers + waitFor, matching
the sibling test at line 409 that already uses this pattern.
Test-only change; no production code touched.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Greptile Summary
This PR ships the full review engine: a multi-pass (diff-risk → cross-file-impact → checks-and-tests) pipeline, adjudication with suppression memory, tool-backed evidence, inline diff UI, and end-to-end PR publication via GitHub review API.
Several issues flagged in previous rounds have been addressed — the
byClassSQL query now correctly deduplicates vialatest_onlyCTE, andPATH_GLOB_CACHE_MAXcaps the suppression glob cache. The two P1 concerns from earlier iterations (diff-position off-by-one inparseDiffFilesandpublishRunpassing the full unslicedchangedFilesinstead of the budget-capped list) remain present and are worth resolving before the publication flow goes to production.Confidence Score: 4/5
Safe to merge for local-only review runs; the PR publication path has two outstanding P1 issues (diff-position off-by-one, full changedFiles list passed to publisher) that will cause systematically misplaced inline comments on GitHub.
The architecture is well-structured and the adjudication/suppression logic is solid. Several previously flagged issues have been addressed (byClass query deduplication, glob cache bounding). Two P1 issues from prior rounds remain open in the publication path: the hunk-header diff-position off-by-one in parseDiffFiles and publishRun passing the full unsliced file list instead of the budget-capped slice.
apps/desktop/src/main/services/review/reviewTargetMaterializer.ts (diff position off-by-one), apps/desktop/src/main/services/review/reviewService.ts (publishRun changedFiles inconsistency)
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as ReviewPage (Renderer) participant IPC as Preload Bridge participant RS as ReviewService (Main) participant RTM as ReviewTargetMaterializer participant RCB as ReviewContextBuilder participant LLM as AI Model participant ADJ as Adjudicator participant SUP as SuppressionService participant PRS as PrService UI->>IPC: startReviewRun(args) IPC->>RS: startRun(args) RS->>RTM: materialize(target, config) RTM-->>RS: ReviewMaterializedTarget (changedFiles, fullPatchText) RS->>RCB: buildContext(run, materialized) RCB-->>RS: ReviewContextPacket (provenance, rules, validation) loop For each pass RS->>LLM: executePass(prompt + diff + context) LLM-->>RS: PassCandidateFinding[] end RS->>ADJ: adjudicate(allCandidates, budgets) ADJ-->>RS: AdjudicationOutcome loop For each finding RS->>SUP: match(finding, repoKey) SUP-->>RS: suppressionMatch or null end alt publishBehavior is auto_publish RS->>PRS: publishReviewPublication(runId, destination, findings, changedFiles) PRS->>PRS: getReviewSnapshot(prId) PRS->>PRS: submitReviewRequest(event, body, inlineComments, commitSha) PRS-->>RS: ReviewPublication RS->>RS: updateFindingPublicationState end RS-->>UI: run-completed eventPrompt To Fix All With AI
Reviews (6): Last reviewed commit: "ship: iteration 5 — fix flaky TerminalVi..." | Re-trigger Greptile