From fb891a003881a6c623e331b12b57fbf79f887105 Mon Sep 17 00:00:00 2001 From: syn Date: Sat, 18 Jul 2026 10:27:44 -0500 Subject: [PATCH 1/2] Add typed Neon review findings API --- .changeset/typed-neon-findings-glow.md | 5 + .plans/DEVIATIONS.md | 7 + shared/review-finding.ts | 91 ++++++ shared/review-surface.ts | 10 +- src/agents/display-assistant.ts | 3 + src/agents/support/tools.ts | 2 + src/modules/review-surfaces/actions.ts | 155 +++++++++ src/modules/review-surfaces/index.ts | 25 ++ src/modules/review-surfaces/registry.ts | 418 +++++++++++++++++++++++- src/modules/review-surfaces/schemas.ts | 182 +++++++++++ src/modules/safety/policy-entries.ts | 60 ++++ src/review-surfaces.test.ts | 366 ++++++++++++++++++++- src/server/events/event-stream.test.ts | 1 + src/server/routes/review-surfaces.ts | 51 +++ 14 files changed, 1372 insertions(+), 4 deletions(-) create mode 100644 .changeset/typed-neon-findings-glow.md create mode 100644 shared/review-finding.ts create mode 100644 src/modules/review-surfaces/actions.ts diff --git a/.changeset/typed-neon-findings-glow.md b/.changeset/typed-neon-findings-glow.md new file mode 100644 index 00000000..9a733a42 --- /dev/null +++ b/.changeset/typed-neon-findings-glow.md @@ -0,0 +1,5 @@ +--- +'neondeck': patch +--- + +Add a revision-bound ephemeral Neon finding contract with targeted review-surface APIs, events, and Flue capabilities. diff --git a/.plans/DEVIATIONS.md b/.plans/DEVIATIONS.md index 88b230a9..6c5316e6 100644 --- a/.plans/DEVIATIONS.md +++ b/.plans/DEVIATIONS.md @@ -22,6 +22,13 @@ Use this format: - Reason: Stable query identity, bounded immutable metadata reuse, active-patch prioritization, and bounded warm thread reuse delivered passing warm first-patch and thread medians and removed duplicate/abandoned work. The remaining misses are measured, isolated follow-ups that do not require overlapping changes in the Phase B review-map/cursor seam, but they must not be represented as passing or erased. - Follow-up: Reprofile the production tree boot/query/render boundary; separate cold network object-fetch time from local metadata before changing refspecs or the <3-second budget; and evaluate uncached GitHub thread latency without weakening cancellation or mutation invalidation. Remeasure the retained immutable real PR before changing any budget, then archive `.plans/PR_REVIEW_PERF_PLAN.md` only after these deferrals are reconciled. +## 2026-07-18 - Diff Review Phase B Finding Backend Split + +- Roadmap item: Diff Improvements Plan / Phase B typed Neon finding application and inline rendering +- Decision: Land the versioned finding contract, process-ephemeral review-surface state, bounded local APIs, targeted events, and Flue tools/actions separately from Pierre/React rendering and explicit promotion into GitHub drafts or prepared-diff revision requests. The shared lifecycle vocabulary includes `resolved` and `promoted`, but this backend slice exposes only apply, read, dismiss, clear, automatic staling, and cleanup transitions. +- Reason: Diff Improvements Phase B is split across parallel backend and review-UI workstreams. Keeping promotion out of the finding application path preserves the trust boundary that applying local context cannot mutate GitHub or prepared diffs. +- Follow-up: The review-surface UI workstream should render these findings inline, add navigation and explicit user-owned promotion controls, and transition resolved/promoted lifecycle state only through the existing typed GitHub draft and prepared-diff revision workflows. + ## 2026-07-17 - Diff Review Phase A Sequencing - Roadmap item: Diff Improvements Plan / transition from Phase A to Phase B diff --git a/shared/review-finding.ts b/shared/review-finding.ts new file mode 100644 index 00000000..06e146ae --- /dev/null +++ b/shared/review-finding.ts @@ -0,0 +1,91 @@ +export const neonReviewFindingSchemaVersion = 1 as const; + +export const neonReviewFindingLimits = { + maxApplyBatch: 50, + maxFindingsPerSurface: 200, + maxTitleLength: 160, + maxExplanationLength: 2_000, + maxSuggestedActionLength: 500, + maxLifecycleReasonLength: 500, + maxEventFindingIds: 50, +} as const; + +export type NeonReviewFindingSeverity = 'critical' | 'major' | 'minor' | 'nit'; + +export type NeonReviewFindingConfidence = 'high' | 'medium' | 'low'; + +export type NeonReviewFindingState = + 'active' | 'stale' | 'resolved' | 'dismissed' | 'promoted'; + +export type NeonReviewFindingSide = 'additions' | 'deletions'; + +export type NeonReviewFindingAnchor = + | { + kind: 'line-range'; + side: NeonReviewFindingSide; + startLine: number; + endLine: number; + } + | { + kind: 'hunk'; + side: NeonReviewFindingSide; + hunkId: string; + }; + +export type NeonReviewFindingProvenance = { + authorRole: string; + model: string | null; + workflowRunId: string | null; + createdAt: string; +}; + +export type NeonReviewFindingLifecycle = { + state: NeonReviewFindingState; + changedAt: string; + reason: string | null; +}; + +export type NeonReviewFinding = { + schemaVersion: typeof neonReviewFindingSchemaVersion; + id: string; + surfaceId: string; + sourceId: string; + revisionKey: string; + file: string; + anchor: NeonReviewFindingAnchor; + title: string; + explanation: string; + severity: NeonReviewFindingSeverity; + confidence: NeonReviewFindingConfidence | null; + suggestedAction: string | null; + provenance: NeonReviewFindingProvenance; + lifecycle: NeonReviewFindingLifecycle; +}; + +export type NeonReviewFindingDraft = Omit< + NeonReviewFinding, + 'surfaceId' | 'provenance' | 'lifecycle' +> & { + provenance: Omit; +}; + +export type ReviewSurfaceFindingsApplyRequest = { + revisionKey: string; + findings: NeonReviewFindingDraft[]; +}; + +export type ReviewSurfaceFindingsDismissRequest = { + findingIds: string[]; + reason: string | null; +}; + +export type ReviewSurfaceFindingsClearRequest = { + findingIds?: string[]; +}; + +export type ReviewSurfaceFindingChange = { + action: 'applied' | 'dismissed' | 'cleared' | 'staled'; + revisionKey: string | null; + findingIds: string[]; + count: number; +}; diff --git a/shared/review-surface.ts b/shared/review-surface.ts index b8e749de..5a3650f4 100644 --- a/shared/review-surface.ts +++ b/shared/review-surface.ts @@ -1,4 +1,5 @@ import type { ReviewSourceSnapshot } from './review-source'; +import type { ReviewSurfaceFindingChange } from './review-finding'; export const reviewSurfaceSchemaVersion = 1 as const; @@ -62,11 +63,18 @@ export type ReviewSurfaceNavigationAck = { export type ReviewSurfaceChangeEvent = { id: string; - action: 'registered' | 'updated' | 'removed' | 'navigation' | 'acknowledged'; + action: + | 'registered' + | 'updated' + | 'removed' + | 'navigation' + | 'acknowledged' + | 'findings-changed'; surfaceId: string; changedAt: string; surface: ActiveReviewSurface | null; navigation: ReviewSurfaceNavigationCommand | null; acknowledgement: ReviewSurfaceNavigationAck | null; + findings: ReviewSurfaceFindingChange | null; reason: 'closed' | 'expired' | null; }; diff --git a/src/agents/display-assistant.ts b/src/agents/display-assistant.ts index 89fa6aec..4f1d0b53 100644 --- a/src/agents/display-assistant.ts +++ b/src/agents/display-assistant.ts @@ -23,6 +23,7 @@ import { } from '../modules/memory'; import { neondeckPrEventActions } from '../modules/pr-events'; import { neondeckPreparedDiffActions } from '../modules/prepared-diffs'; +import { neondeckReviewSurfaceActions } from '../modules/review-surfaces'; import { neondeckRuntimeSkillActions, runtimeSkillReferencesSync, @@ -72,6 +73,7 @@ export default defineAgent(({ id }) => { 'For morning briefings, use neondeck_briefing_profile_read, neondeck_briefing_profile_update, neondeck_briefing_run_now, and neondeck_briefing_run_read for exact persisted grounding. Briefing instructions may request any configured MCP source, but do not add a briefing-specific allowlist or auto-approve mutations. For other scheduled work, use neondeck_scheduled_task_instruction_create. Scheduled instructions run as bounded workflows by default; select an explicit agent session target only when the user needs continuity. Use the scheduled-task list, read, pause, resume, and delete actions to operate existing tasks.', 'For autopilot status, use neondeck_autopilot_state_lookup before explaining what Neon is watching, why it did or did not act, which worktrees are prepared, which approvals are pending, and what repo/watch policy allows. Treat this as read-only operator state; do not invent queue entries, diffs, pushes, or workflow outcomes that are not present in the lookup.', 'For prepared autopilot diffs, use neondeck_prepared_diff_list, neondeck_prepared_diff_summary, neondeck_prepared_diff_changed_files, and neondeck_prepared_diff_file_diff for facts. Use neondeck_autopilot_recovery_options before recommending recovery from prepared, blocked, pushed, or failed autopilot states. Use neondeck_autopilot_recovery_run for bounded inspect, retry-after-new-commit, rebase/resync worktree, retry verify, retry push, retry comment, request revision, cleanup worktree, abandon, or manual-follow-up decisions. These recovery actions dispatch to existing prepared-diff, worktree sync/cleanup, and autopilot services, keep the source worktree as the source of truth, and never bypass confirmation, execution, policy, cleanup, or GitHub gates.', + 'For a live diff, use neondeck_review_surfaces_lookup to resolve the intended window and neondeck_review_surface_context_lookup to read its bounded source metadata and ephemeral findings. Use neondeck_review_surface_navigate only for one explicit surface id, keeping focus=false unless the user asked to be shown the target. Apply findings only through neondeck_review_surface_findings_apply with the exact active source and revision, stable ids, concise bounded text, and provenance; dismiss or clear them explicitly. Review-surface findings are process-ephemeral local context only and never create GitHub comments, submit reviews, request prepared-diff revisions, or silently re-anchor across revisions.', 'The neondeck_autopilot_prepare_pr_worktree, neondeck_autopilot_fix_pr_review_feedback, neondeck_autopilot_fix_pr_ci_failure, and neondeck_autopilot_push_pr_autofix actions are workflow-only and refuse in interactive chat. Autonomous watcher workflows use them under a Flue run id and remain governed by the configured per-PR mode.', 'For PR event facts and watermarks, use neondeck_github_pr_event_state_get, neondeck_pr_review_comments_lookup, neondeck_pr_requested_changes_lookup, neondeck_pr_branch_permissions_lookup, neondeck_pr_watch_event_state_refresh, and neondeck_pr_watch_event_watermarks_list. These GitHub fact collectors and app-state watermarks do not prepare fixes or push. Use neondeck_pr_comment only when the intended PR comment text is explicit and grounded in deterministic facts; prefer neondeck_autopilot_comment_pr_autofix_result for autonomous prepared-diff result comments because it also records the workflow summary audit.', 'For quick deterministic facts, prefer the neondeck_*_lookup tools. Use actions when you need a durable command, mutation, scheduler tick, or persisted workflow summary.', @@ -120,6 +122,7 @@ export default defineAgent(({ id }) => { ...neondeckAutopilotActions, ...neondeckAutopilotRecoveryActions, ...neondeckPreparedDiffActions, + ...neondeckReviewSurfaceActions, ...neondeckScheduledTaskActions, ...neondeckSchedulerActions, ...neondeckSessionActions, diff --git a/src/agents/support/tools.ts b/src/agents/support/tools.ts index 25f7f1a1..24549289 100644 --- a/src/agents/support/tools.ts +++ b/src/agents/support/tools.ts @@ -20,6 +20,7 @@ import { neondeckMcpTools } from '../../domains/mcp'; import { neondeckPrEventTools } from '../../modules/pr-events'; import { neondeckPreparedDiffTools } from '../../modules/prepared-diffs'; import { neondeckRepoEditTools } from '../../repo-edit'; +import { neondeckReviewSurfaceTools } from '../../modules/review-surfaces'; import { listRepoStatus, listRuntimeSkills, @@ -339,6 +340,7 @@ export const neondeckFactTools = [ safetyPolicyTool, executionPolicyTool, ...neondeckPrEventTools, + ...neondeckReviewSurfaceTools, ...neondeckRepoEditTools, ...neondeckWorktreeTools, ...neondeckKiloTools, diff --git a/src/modules/review-surfaces/actions.ts b/src/modules/review-surfaces/actions.ts new file mode 100644 index 00000000..20f48b6e --- /dev/null +++ b/src/modules/review-surfaces/actions.ts @@ -0,0 +1,155 @@ +import { defineAction, defineTool } from '@flue/runtime'; +import * as v from 'valibot'; +import { currentFlueExecutionContext } from '../flue'; +import { reviewSurfaceRegistry } from './registry'; +import { + reviewSurfaceActionOutputSchema, + reviewSurfaceFindingsApplyActionSchema, + reviewSurfaceFindingsClearActionSchema, + reviewSurfaceFindingsDismissActionSchema, + reviewSurfaceIdInputSchema, + reviewSurfaceNavigateInputSchema, +} from './schemas'; + +const lookupOutputSchema = v.looseObject({ ok: v.boolean() }); + +export const reviewSurfacesLookupTool = defineTool({ + name: 'neondeck_review_surfaces_lookup', + description: + 'List concise metadata for active process-ephemeral review surfaces without patch bodies or finding text.', + input: v.object({}), + output: lookupOutputSchema, + async run() { + return { + ok: true, + surfaces: reviewSurfaceRegistry.list().map((surface) => ({ + surfaceId: surface.surfaceId, + source: { + id: surface.source.id, + kind: surface.source.kind, + title: surface.source.title, + revision: surface.source.revision, + }, + activePath: surface.activePath, + updatedAt: surface.updatedAt, + expiresAt: surface.expiresAt, + findingCount: + reviewSurfaceRegistry.readFindings(surface.surfaceId).count ?? 0, + })), + }; + }, +}); + +export const reviewSurfaceContextLookupTool = defineTool({ + name: 'neondeck_review_surface_context_lookup', + description: + 'Read one active review surface and its bounded process-ephemeral Neon findings. This does not load patch bodies.', + input: reviewSurfaceIdInputSchema, + output: lookupOutputSchema, + async run({ input }) { + const surface = reviewSurfaceRegistry.read(input.surfaceId); + if (!surface) { + return { + ok: false, + message: 'Review surface is not active.', + surfaceId: input.surfaceId, + }; + } + return { + ...reviewSurfaceRegistry.readFindings(input.surfaceId), + surface, + }; + }, +}); + +export const reviewSurfaceNavigateAction = defineAction({ + name: 'neondeck_review_surface_navigate', + description: + 'Publish a revision-aware file navigation command to exactly one active review surface. Set focus only when the user explicitly asked to be shown the target.', + input: reviewSurfaceNavigateInputSchema, + output: reviewSurfaceActionOutputSchema, + async run({ input }) { + const navigation = reviewSurfaceRegistry.navigate(input.surfaceId, { + revisionKey: input.revisionKey, + target: input.target, + }); + if (!navigation) { + return { + ok: false, + action: 'navigate', + changed: false, + message: 'Review surface is not active.', + surfaceId: input.surfaceId, + }; + } + return { + ok: true, + action: 'navigate', + changed: true, + message: 'Published a targeted review surface navigation command.', + surfaceId: input.surfaceId, + revisionKey: navigation.revisionKey, + navigation, + }; + }, +}); + +export const reviewSurfaceFindingsApplyAction = defineAction({ + name: 'neondeck_review_surface_findings_apply', + description: + 'Atomically apply a bounded batch of revision-bound, process-ephemeral Neon findings to one active review surface. This never creates GitHub comments or mutates prepared diffs.', + input: reviewSurfaceFindingsApplyActionSchema, + output: reviewSurfaceActionOutputSchema, + async run({ input }) { + const workflowRunId = currentFlueExecutionContext()?.runId ?? null; + return reviewSurfaceRegistry.applyFindings(input.surfaceId, { + revisionKey: input.revisionKey, + findings: input.findings.map((finding) => ({ + ...finding, + provenance: { + ...finding.provenance, + workflowRunId: workflowRunId ?? finding.provenance.workflowRunId, + }, + })), + }); + }, +}); + +export const reviewSurfaceFindingsDismissAction = defineAction({ + name: 'neondeck_review_surface_findings_dismiss', + description: + 'Explicitly dismiss selected process-ephemeral Neon findings on one active review surface.', + input: reviewSurfaceFindingsDismissActionSchema, + output: reviewSurfaceActionOutputSchema, + async run({ input }) { + return reviewSurfaceRegistry.dismissFindings(input.surfaceId, { + findingIds: input.findingIds, + reason: input.reason, + }); + }, +}); + +export const reviewSurfaceFindingsClearAction = defineAction({ + name: 'neondeck_review_surface_findings_clear', + description: + 'Explicitly remove selected or all process-ephemeral Neon findings from one active review surface.', + input: reviewSurfaceFindingsClearActionSchema, + output: reviewSurfaceActionOutputSchema, + async run({ input }) { + return reviewSurfaceRegistry.clearFindings(input.surfaceId, { + findingIds: input.findingIds, + }); + }, +}); + +export const neondeckReviewSurfaceTools = [ + reviewSurfacesLookupTool, + reviewSurfaceContextLookupTool, +]; + +export const neondeckReviewSurfaceActions = [ + reviewSurfaceNavigateAction, + reviewSurfaceFindingsApplyAction, + reviewSurfaceFindingsDismissAction, + reviewSurfaceFindingsClearAction, +]; diff --git a/src/modules/review-surfaces/index.ts b/src/modules/review-surfaces/index.ts index 69668beb..4088b19c 100644 --- a/src/modules/review-surfaces/index.ts +++ b/src/modules/review-surfaces/index.ts @@ -4,8 +4,33 @@ export { reviewSurfaceTtlMs, ReviewSurfaceRegistry, } from './registry'; +export type { + ReviewSurfaceFindingErrorCode, + ReviewSurfaceFindingResult, +} from './registry'; export { + neonReviewFindingDraftSchema, + neonReviewFindingSchema, + reviewSurfaceActionOutputSchema, + reviewSurfaceFindingsApplyActionSchema, + reviewSurfaceFindingsApplySchema, + reviewSurfaceFindingsClearActionSchema, + reviewSurfaceFindingsClearSchema, + reviewSurfaceFindingsDismissActionSchema, + reviewSurfaceFindingsDismissSchema, + reviewSurfaceIdInputSchema, reviewSurfaceNavigationAckInputSchema, + reviewSurfaceNavigateInputSchema, reviewSurfaceNavigationRequestSchema, reviewSurfaceSnapshotSchema, } from './schemas'; +export { + neondeckReviewSurfaceActions, + neondeckReviewSurfaceTools, + reviewSurfaceContextLookupTool, + reviewSurfaceFindingsApplyAction, + reviewSurfaceFindingsClearAction, + reviewSurfaceFindingsDismissAction, + reviewSurfaceNavigateAction, + reviewSurfacesLookupTool, +} from './actions'; diff --git a/src/modules/review-surfaces/registry.ts b/src/modules/review-surfaces/registry.ts index 7b898ee8..90a33fb3 100644 --- a/src/modules/review-surfaces/registry.ts +++ b/src/modules/review-surfaces/registry.ts @@ -1,4 +1,13 @@ import { randomUUID } from 'node:crypto'; +import { + neonReviewFindingLimits, + type NeonReviewFinding, + type NeonReviewFindingDraft, + type ReviewSurfaceFindingChange, + type ReviewSurfaceFindingsApplyRequest, + type ReviewSurfaceFindingsClearRequest, + type ReviewSurfaceFindingsDismissRequest, +} from '../../../shared/review-finding'; import { reviewRevisionKey } from '../../../shared/review-source'; import type { ActiveReviewSurface, @@ -19,7 +28,35 @@ type ReviewSurfaceRegistryOptions = { ttlMs?: number; }; +export type ReviewSurfaceFindingErrorCode = + | 'surface-not-active' + | 'revision-unavailable' + | 'stale-revision' + | 'source-mismatch' + | 'file-unavailable' + | 'invalid-batch-size' + | 'duplicate-finding-id' + | 'finding-id-conflict' + | 'surface-finding-limit'; + +export type ReviewSurfaceFindingResult = { + ok: boolean; + action: string; + changed: boolean; + message: string; + surfaceId: string; + revisionKey?: string | null; + findings?: NeonReviewFinding[]; + findingIds?: string[]; + count?: number; + error?: { + code: ReviewSurfaceFindingErrorCode; + message: string; + }; +}; + export class ReviewSurfaceRegistry { + private readonly findings = new Map>(); private readonly listeners = new Set(); private readonly pendingNavigations = new Map(); private readonly records = new Map(); @@ -48,6 +85,21 @@ export class ReviewSurfaceRegistry { action: existing ? 'updated' : 'registered', surfaceId: surface.surfaceId, }); + const previousRevisionKey = existing + ? reviewRevisionKey(existing.source.revision) + : null; + const nextRevisionKey = reviewRevisionKey(surface.source.revision); + if ( + existing && + (existing.source.id !== surface.source.id || + previousRevisionKey !== nextRevisionKey) + ) { + this.staleActiveFindings( + surface.surfaceId, + nextRevisionKey, + 'The review surface source or revision changed.', + ); + } this.scheduleExpiration(); return surface; } @@ -64,6 +116,242 @@ export class ReviewSurfaceRegistry { return this.records.get(surfaceId) ?? null; } + readFindings(surfaceId: string): ReviewSurfaceFindingResult { + const surface = this.read(surfaceId); + if (!surface) + return this.findingError(surfaceId, 'read', 'surface-not-active'); + const findings = [...(this.findings.get(surfaceId)?.values() ?? [])].sort( + (left, right) => + left.provenance.createdAt.localeCompare(right.provenance.createdAt) || + left.id.localeCompare(right.id), + ); + return { + ok: true, + action: 'read', + changed: false, + message: `Read ${findings.length} ephemeral review finding(s).`, + surfaceId, + revisionKey: reviewRevisionKey(surface.source.revision), + findings, + count: findings.length, + }; + } + + applyFindings( + surfaceId: string, + input: ReviewSurfaceFindingsApplyRequest, + ): ReviewSurfaceFindingResult { + const surface = this.read(surfaceId); + if (!surface) + return this.findingError(surfaceId, 'apply', 'surface-not-active'); + const currentRevisionKey = reviewRevisionKey(surface.source.revision); + if (!currentRevisionKey) { + return this.findingError(surfaceId, 'apply', 'revision-unavailable'); + } + if (input.revisionKey !== currentRevisionKey) { + return this.findingError(surfaceId, 'apply', 'stale-revision', { + revisionKey: currentRevisionKey, + }); + } + + if ( + input.findings.length === 0 || + input.findings.length > neonReviewFindingLimits.maxApplyBatch + ) { + return this.findingError(surfaceId, 'apply', 'invalid-batch-size', { + revisionKey: currentRevisionKey, + }); + } + + const ids = input.findings.map((finding) => finding.id); + if (new Set(ids).size !== ids.length) { + return this.findingError(surfaceId, 'apply', 'duplicate-finding-id', { + revisionKey: currentRevisionKey, + }); + } + + const sourcePaths = new Set(surface.source.files.map((file) => file.path)); + for (const finding of input.findings) { + if ( + finding.sourceId !== surface.source.id || + finding.revisionKey !== currentRevisionKey + ) { + return this.findingError(surfaceId, 'apply', 'source-mismatch', { + revisionKey: currentRevisionKey, + }); + } + if (!sourcePaths.has(finding.file)) { + return this.findingError(surfaceId, 'apply', 'file-unavailable', { + revisionKey: currentRevisionKey, + }); + } + } + + const existing = this.findings.get(surfaceId) ?? new Map(); + for (const finding of input.findings) { + const current = existing.get(finding.id); + if (current && !sameFindingDraft(current, finding)) { + return this.findingError(surfaceId, 'apply', 'finding-id-conflict', { + revisionKey: currentRevisionKey, + }); + } + } + + const newFindings = input.findings.filter( + (finding) => !existing.has(finding.id), + ); + if ( + existing.size + newFindings.length > + neonReviewFindingLimits.maxFindingsPerSurface + ) { + return this.findingError(surfaceId, 'apply', 'surface-finding-limit', { + revisionKey: currentRevisionKey, + }); + } + + if (newFindings.length === 0) { + return { + ok: true, + action: 'apply', + changed: false, + message: 'All finding ids were already applied with identical content.', + surfaceId, + revisionKey: currentRevisionKey, + findings: input.findings.map((finding) => existing.get(finding.id)!), + findingIds: [], + count: 0, + }; + } + + const changedAt = this.timestamp(); + const applied = newFindings.map((finding) => + materializeFinding(surfaceId, finding, changedAt), + ); + const next = new Map(existing); + for (const finding of applied) next.set(finding.id, finding); + this.findings.set(surfaceId, next); + this.publishFindingChange(surfaceId, { + action: 'applied', + revisionKey: currentRevisionKey, + findingIds: applied.map((finding) => finding.id), + count: applied.length, + }); + return { + ok: true, + action: 'apply', + changed: true, + message: `Applied ${applied.length} ephemeral review finding(s).`, + surfaceId, + revisionKey: currentRevisionKey, + findings: input.findings.map((finding) => next.get(finding.id)!), + findingIds: applied.map((finding) => finding.id), + count: applied.length, + }; + } + + dismissFindings( + surfaceId: string, + input: ReviewSurfaceFindingsDismissRequest, + ): ReviewSurfaceFindingResult { + const surface = this.read(surfaceId); + if (!surface) + return this.findingError(surfaceId, 'dismiss', 'surface-not-active'); + if ( + input.findingIds.length === 0 || + input.findingIds.length > neonReviewFindingLimits.maxApplyBatch + ) { + return this.findingError(surfaceId, 'dismiss', 'invalid-batch-size'); + } + if (new Set(input.findingIds).size !== input.findingIds.length) { + return this.findingError(surfaceId, 'dismiss', 'duplicate-finding-id'); + } + const current = this.findings.get(surfaceId); + const changedAt = this.timestamp(); + const changedIds: string[] = []; + const next = new Map(current); + for (const findingId of input.findingIds) { + const finding = next.get(findingId); + if (!finding || finding.lifecycle.state === 'dismissed') continue; + next.set(findingId, { + ...finding, + lifecycle: { + state: 'dismissed', + changedAt, + reason: input.reason, + }, + }); + changedIds.push(findingId); + } + if (changedIds.length > 0) { + this.findings.set(surfaceId, next); + this.publishFindingChange(surfaceId, { + action: 'dismissed', + revisionKey: reviewRevisionKey(surface.source.revision), + findingIds: changedIds, + count: changedIds.length, + }); + } + return { + ok: true, + action: 'dismiss', + changed: changedIds.length > 0, + message: `Dismissed ${changedIds.length} ephemeral review finding(s).`, + surfaceId, + revisionKey: reviewRevisionKey(surface.source.revision), + findingIds: changedIds, + count: changedIds.length, + }; + } + + clearFindings( + surfaceId: string, + input: ReviewSurfaceFindingsClearRequest, + ): ReviewSurfaceFindingResult { + const surface = this.read(surfaceId); + if (!surface) + return this.findingError(surfaceId, 'clear', 'surface-not-active'); + if ( + input.findingIds && + (input.findingIds.length === 0 || + input.findingIds.length > neonReviewFindingLimits.maxApplyBatch) + ) { + return this.findingError(surfaceId, 'clear', 'invalid-batch-size'); + } + if ( + input.findingIds && + new Set(input.findingIds).size !== input.findingIds.length + ) { + return this.findingError(surfaceId, 'clear', 'duplicate-finding-id'); + } + const current = this.findings.get(surfaceId); + const requestedIds = input.findingIds ?? [...(current?.keys() ?? [])]; + const clearedIds = requestedIds.filter((findingId) => + current?.has(findingId), + ); + if (clearedIds.length > 0 && current) { + const next = new Map(current); + for (const findingId of clearedIds) next.delete(findingId); + if (next.size > 0) this.findings.set(surfaceId, next); + else this.findings.delete(surfaceId); + this.publishFindingChange(surfaceId, { + action: 'cleared', + revisionKey: reviewRevisionKey(surface.source.revision), + findingIds: clearedIds, + count: clearedIds.length, + }); + } + return { + ok: true, + action: 'clear', + changed: clearedIds.length > 0, + message: `Cleared ${clearedIds.length} ephemeral review finding(s).`, + surfaceId, + revisionKey: reviewRevisionKey(surface.source.revision), + findingIds: clearedIds, + count: clearedIds.length, + }; + } + heartbeat(surfaceId: string) { const current = this.read(surfaceId); if (!current) return null; @@ -80,6 +368,7 @@ export class ReviewSurfaceRegistry { const surface = this.records.get(surfaceId); if (!surface) return false; this.records.delete(surfaceId); + this.findings.delete(surfaceId); for (const [commandId, targetSurfaceId] of this.pendingNavigations) { if (targetSurfaceId === surfaceId) this.pendingNavigations.delete(commandId); @@ -160,6 +449,7 @@ export class ReviewSurfaceRegistry { this.listeners.clear(); this.pendingNavigations.clear(); this.records.clear(); + this.findings.clear(); } private pruneExpired() { @@ -195,7 +485,7 @@ export class ReviewSurfaceRegistry { Partial< Pick< ReviewSurfaceChangeEvent, - 'surface' | 'navigation' | 'acknowledgement' | 'reason' + 'surface' | 'navigation' | 'acknowledgement' | 'reason' | 'findings' > >, ) { @@ -207,6 +497,7 @@ export class ReviewSurfaceRegistry { surface: input.surface ?? null, navigation: input.navigation ?? null, acknowledgement: input.acknowledgement ?? null, + findings: input.findings ?? null, reason: input.reason ?? null, }; for (const listener of this.listeners) { @@ -222,6 +513,131 @@ export class ReviewSurfaceRegistry { private timestamp() { return new Date(this.now()).toISOString(); } + + private staleActiveFindings( + surfaceId: string, + revisionKey: string | null, + reason: string, + ) { + const current = this.findings.get(surfaceId); + if (!current) return; + const changedAt = this.timestamp(); + const changedIds: string[] = []; + const next = new Map(current); + for (const [findingId, finding] of current) { + if (finding.lifecycle.state !== 'active') continue; + next.set(findingId, { + ...finding, + lifecycle: { state: 'stale', changedAt, reason }, + }); + changedIds.push(findingId); + } + if (changedIds.length === 0) return; + this.findings.set(surfaceId, next); + this.publishFindingChange(surfaceId, { + action: 'staled', + revisionKey, + findingIds: changedIds, + count: changedIds.length, + }); + } + + private publishFindingChange( + surfaceId: string, + change: ReviewSurfaceFindingChange, + ) { + this.publish({ + action: 'findings-changed', + surfaceId, + findings: { + ...change, + findingIds: change.findingIds.slice( + 0, + neonReviewFindingLimits.maxEventFindingIds, + ), + }, + }); + } + + private findingError( + surfaceId: string, + action: string, + code: ReviewSurfaceFindingErrorCode, + extra: Pick = {}, + ): ReviewSurfaceFindingResult { + const message = findingErrorMessage(code); + return { + ok: false, + action, + changed: false, + message, + surfaceId, + ...extra, + error: { code, message }, + }; + } +} + +function materializeFinding( + surfaceId: string, + finding: NeonReviewFindingDraft, + createdAt: string, +): NeonReviewFinding { + return { + ...finding, + surfaceId, + provenance: { ...finding.provenance, createdAt }, + lifecycle: { state: 'active', changedAt: createdAt, reason: null }, + }; +} + +function sameFindingDraft( + finding: NeonReviewFinding, + draft: NeonReviewFindingDraft, +) { + return ( + JSON.stringify({ + schemaVersion: finding.schemaVersion, + id: finding.id, + sourceId: finding.sourceId, + revisionKey: finding.revisionKey, + file: finding.file, + anchor: finding.anchor, + title: finding.title, + explanation: finding.explanation, + severity: finding.severity, + confidence: finding.confidence, + suggestedAction: finding.suggestedAction, + provenance: { + authorRole: finding.provenance.authorRole, + model: finding.provenance.model, + workflowRunId: finding.provenance.workflowRunId, + }, + }) === JSON.stringify(draft) + ); +} + +function findingErrorMessage(code: ReviewSurfaceFindingErrorCode) { + switch (code) { + case 'surface-not-active': + return 'Review surface is not active.'; + case 'revision-unavailable': + return 'Review surface has no resolved revision for anchored findings.'; + case 'stale-revision': + return 'Finding batch revision does not match the active review surface.'; + case 'source-mismatch': + return 'Every finding must match the active source and revision.'; + case 'file-unavailable': + return 'Every finding must anchor to a file in the active review surface.'; + case 'invalid-batch-size': + return `Finding batches must contain between 1 and ${neonReviewFindingLimits.maxApplyBatch} items.`; + case 'duplicate-finding-id': + return 'Finding ids must be unique within a batch.'; + case 'finding-id-conflict': + return 'A finding id is already associated with different content.'; + case 'surface-finding-limit': + return `A review surface may retain at most ${neonReviewFindingLimits.maxFindingsPerSurface} ephemeral findings.`; + } } export const reviewSurfaceRegistry = new ReviewSurfaceRegistry(); diff --git a/src/modules/review-surfaces/schemas.ts b/src/modules/review-surfaces/schemas.ts index 23a8a70b..098ca26d 100644 --- a/src/modules/review-surfaces/schemas.ts +++ b/src/modules/review-surfaces/schemas.ts @@ -1,4 +1,13 @@ import * as v from 'valibot'; +import { + neonReviewFindingLimits, + neonReviewFindingSchemaVersion, + type NeonReviewFinding, + type NeonReviewFindingDraft, + type ReviewSurfaceFindingsApplyRequest, + type ReviewSurfaceFindingsClearRequest, + type ReviewSurfaceFindingsDismissRequest, +} from '../../../shared/review-finding'; import { reviewSourceSchemaVersion, type ReviewSourceSnapshot, @@ -8,12 +17,22 @@ import { type ReviewSurfaceNavigationRequest, type ReviewSurfaceSnapshot, } from '../../../shared/review-surface'; +import { isoDateStringSchema } from '../../lib/valibot'; const identifierSchema = v.pipe(v.string(), v.minLength(1), v.maxLength(240)); const surfaceIdSchema = v.pipe(v.string(), v.minLength(1), v.maxLength(512)); const revisionKeySchema = v.pipe(v.string(), v.minLength(1), v.maxLength(768)); const pathSchema = v.pipe(v.string(), v.minLength(1), v.maxLength(4_096)); const nullableIdentifierSchema = v.nullable(identifierSchema); +const findingIdListSchema = v.pipe( + v.array(identifierSchema), + v.minLength(1), + v.maxLength(neonReviewFindingLimits.maxApplyBatch), + v.check( + (findingIds) => new Set(findingIds).size === findingIds.length, + 'Finding ids must be unique.', + ), +); const reviewRevisionSchema = v.variant('state', [ v.object({ @@ -130,3 +149,166 @@ export const reviewSurfaceNavigationAckInputSchema = v.object({ resolvedPath: v.nullable(pathSchema), message: v.nullable(v.pipe(v.string(), v.maxLength(500))), }); + +const findingSideSchema = v.picklist(['additions', 'deletions']); +const findingAnchorSchema = v.variant('kind', [ + v.pipe( + v.object({ + kind: v.literal('line-range'), + side: findingSideSchema, + startLine: v.pipe(v.number(), v.integer(), v.minValue(1)), + endLine: v.pipe(v.number(), v.integer(), v.minValue(1)), + }), + v.check( + (anchor) => anchor.endLine >= anchor.startLine, + 'Finding end line must not precede its start line.', + ), + ), + v.object({ + kind: v.literal('hunk'), + side: findingSideSchema, + hunkId: identifierSchema, + }), +]); +const findingSeveritySchema = v.picklist(['critical', 'major', 'minor', 'nit']); +const findingConfidenceSchema = v.nullable( + v.picklist(['high', 'medium', 'low']), +); +const findingProvenanceInputSchema = v.object({ + authorRole: identifierSchema, + model: nullableIdentifierSchema, + workflowRunId: nullableIdentifierSchema, +}); + +const neonReviewFindingDraftEntries = { + schemaVersion: v.literal(neonReviewFindingSchemaVersion), + id: identifierSchema, + sourceId: identifierSchema, + revisionKey: revisionKeySchema, + file: pathSchema, + anchor: findingAnchorSchema, + title: v.pipe( + v.string(), + v.minLength(1), + v.maxLength(neonReviewFindingLimits.maxTitleLength), + ), + explanation: v.pipe( + v.string(), + v.minLength(1), + v.maxLength(neonReviewFindingLimits.maxExplanationLength), + ), + severity: findingSeveritySchema, + confidence: findingConfidenceSchema, + suggestedAction: v.nullable( + v.pipe( + v.string(), + v.minLength(1), + v.maxLength(neonReviewFindingLimits.maxSuggestedActionLength), + ), + ), + provenance: findingProvenanceInputSchema, +}; + +export const neonReviewFindingDraftSchema: v.GenericSchema = + v.object(neonReviewFindingDraftEntries); + +export const neonReviewFindingSchema: v.GenericSchema = + v.object({ + ...neonReviewFindingDraftEntries, + surfaceId: surfaceIdSchema, + provenance: v.object({ + ...findingProvenanceInputSchema.entries, + createdAt: isoDateStringSchema, + }), + lifecycle: v.object({ + state: v.picklist([ + 'active', + 'stale', + 'resolved', + 'dismissed', + 'promoted', + ]), + changedAt: isoDateStringSchema, + reason: v.nullable( + v.pipe( + v.string(), + v.minLength(1), + v.maxLength(neonReviewFindingLimits.maxLifecycleReasonLength), + ), + ), + }), + }); + +const findingBatchSchema = v.pipe( + v.array(neonReviewFindingDraftSchema), + v.minLength(1), + v.maxLength(neonReviewFindingLimits.maxApplyBatch), + v.check( + (findings) => + new Set(findings.map((finding) => finding.id)).size === findings.length, + 'Finding ids must be unique within a batch.', + ), +); + +export const reviewSurfaceFindingsApplySchema: v.GenericSchema = + v.object({ + revisionKey: revisionKeySchema, + findings: findingBatchSchema, + }); + +export const reviewSurfaceFindingsDismissSchema: v.GenericSchema = + v.object({ + findingIds: findingIdListSchema, + reason: v.nullable( + v.pipe( + v.string(), + v.minLength(1), + v.maxLength(neonReviewFindingLimits.maxLifecycleReasonLength), + ), + ), + }); + +export const reviewSurfaceFindingsClearSchema: v.GenericSchema = + v.object({ + findingIds: v.optional(findingIdListSchema), + }); + +export const reviewSurfaceIdInputSchema = v.object({ + surfaceId: surfaceIdSchema, +}); + +export const reviewSurfaceNavigateInputSchema = v.object({ + surfaceId: surfaceIdSchema, + revisionKey: v.nullable(revisionKeySchema), + target: v.object({ path: pathSchema, focus: v.boolean() }), +}); + +export const reviewSurfaceFindingsApplyActionSchema = v.object({ + surfaceId: surfaceIdSchema, + revisionKey: revisionKeySchema, + findings: findingBatchSchema, +}); + +export const reviewSurfaceFindingsDismissActionSchema = v.object({ + surfaceId: surfaceIdSchema, + findingIds: findingIdListSchema, + reason: v.nullable( + v.pipe( + v.string(), + v.minLength(1), + v.maxLength(neonReviewFindingLimits.maxLifecycleReasonLength), + ), + ), +}); + +export const reviewSurfaceFindingsClearActionSchema = v.object({ + surfaceId: surfaceIdSchema, + findingIds: v.optional(findingIdListSchema), +}); + +export const reviewSurfaceActionOutputSchema = v.looseObject({ + ok: v.boolean(), + action: v.string(), + changed: v.boolean(), + message: v.string(), +}); diff --git a/src/modules/safety/policy-entries.ts b/src/modules/safety/policy-entries.ts index c8deb80d..03f7c345 100644 --- a/src/modules/safety/policy-entries.ts +++ b/src/modules/safety/policy-entries.ts @@ -218,6 +218,18 @@ export const entries: SafetyPolicyEntry[] = [ readOnly, 'Reads prepared-diff records and pending decision rows without reading file patches.', ), + tool( + 'neondeck_review_surfaces_lookup', + 'List active review surfaces', + readOnly, + 'Lists concise process-ephemeral review surface identity, revision, focus, expiry, and finding counts without patch bodies or finding text.', + ), + tool( + 'neondeck_review_surface_context_lookup', + 'Read review surface context', + readOnly, + 'Reads one bounded process-ephemeral review surface snapshot and its associated local findings without patch bodies.', + ), tool( 'neondeck_kilo_tasks_lookup', 'Read Kilo handoff tasks', @@ -236,6 +248,30 @@ export const entries: SafetyPolicyEntry[] = [ readOnly, 'Classifies a proposed local or exe.dev command against execution approval policy without running it.', ), + action( + 'neondeck_review_surface_navigate', + 'Navigate active review surface', + unauditedSafeMutation, + 'Publishes one revision-aware targeted navigation event to an active review surface without changing external or durable state.', + ), + action( + 'neondeck_review_surface_findings_apply', + 'Apply review surface findings', + unauditedSafeMutation, + 'Atomically applies bounded revision-bound findings to process-ephemeral surface state. It cannot create GitHub comments or mutate prepared diffs.', + ), + action( + 'neondeck_review_surface_findings_dismiss', + 'Dismiss review surface findings', + unauditedSafeMutation, + 'Marks selected process-ephemeral findings dismissed on one active review surface.', + ), + action( + 'neondeck_review_surface_findings_clear', + 'Clear review surface findings', + unauditedSafeMutation, + 'Explicitly removes selected or all process-ephemeral findings from one active review surface.', + ), action( 'neondeck_execution_request_approval', 'Request host execution approval', @@ -937,6 +973,30 @@ export const entries: SafetyPolicyEntry[] = [ unauditedSafeMutation, 'Extends one process-ephemeral review surface lease without rebroadcasting its context snapshot.', ), + route( + '/api/review-surfaces/:surfaceId/findings', + 'Read review surface findings API', + readOnly, + 'Reads bounded process-ephemeral Neon findings for one active review surface. It does not read patch bodies or durable review data.', + ), + route( + '/api/review-surfaces/:surfaceId/findings/apply', + 'Apply review surface findings API', + unauditedSafeMutation, + 'Atomically applies revision-bound findings to one process-ephemeral review surface. It cannot create GitHub comments or mutate prepared diffs.', + ), + route( + '/api/review-surfaces/:surfaceId/findings/dismiss', + 'Dismiss review surface findings API', + unauditedSafeMutation, + 'Marks selected process-ephemeral findings dismissed on one active review surface.', + ), + route( + '/api/review-surfaces/:surfaceId/findings/clear', + 'Clear review surface findings API', + unauditedSafeMutation, + 'Explicitly removes selected or all process-ephemeral findings from one active review surface.', + ), route( '/api/review-surfaces/:surfaceId/navigation', 'Navigate review surface API', diff --git a/src/review-surfaces.test.ts b/src/review-surfaces.test.ts index 1f75265e..c8982427 100644 --- a/src/review-surfaces.test.ts +++ b/src/review-surfaces.test.ts @@ -1,5 +1,10 @@ import { Hono } from 'hono'; import { afterEach, describe, expect, it } from 'vitest'; +import { + neonReviewFindingLimits, + neonReviewFindingSchemaVersion, + type NeonReviewFindingDraft, +} from '../shared/review-finding'; import { reviewSourceSchemaVersion, resolvedReviewRevision, @@ -163,6 +168,312 @@ describe('review surface registry', () => { message: 'Surface id does not match the route.', }); }); + + it('rejects an invalid finding batch atomically before applying any item', async () => { + const { app, registry } = harness(); + const events: ReviewSurfaceChangeEvent[] = []; + registry.subscribe((event) => events.push(event)); + await register(app, snapshot('surface-a')); + + const response = await apply(app, 'surface-a', [ + finding('finding-valid'), + finding('finding-invalid', { file: 'src/missing.ts' }), + ]); + + expect(response.status).toBe(409); + await expect(response.json()).resolves.toMatchObject({ + ok: false, + changed: false, + error: { code: 'file-unavailable' }, + }); + expect(registry.readFindings('surface-a')).toMatchObject({ + ok: true, + findings: [], + count: 0, + }); + expect(events.map((event) => event.action)).toEqual(['registered']); + }); + + it('stales active findings on a revision change and rejects the old revision', async () => { + const { app, registry } = harness(); + const events: ReviewSurfaceChangeEvent[] = []; + registry.subscribe((event) => events.push(event)); + await register(app, snapshot('surface-a')); + expect((await apply(app, 'surface-a', [finding('finding-a')])).status).toBe( + 200, + ); + + await register(app, snapshot('surface-a', 'next-head-sha')); + + expect(registry.readFindings('surface-a')).toMatchObject({ + findings: [ + { + id: 'finding-a', + revisionKey: 'git-commit::head-sha', + lifecycle: { + state: 'stale', + reason: 'The review surface source or revision changed.', + }, + }, + ], + }); + expect(events.at(-1)).toMatchObject({ + action: 'findings-changed', + surfaceId: 'surface-a', + findings: { + action: 'staled', + revisionKey: 'git-commit::next-head-sha', + findingIds: ['finding-a'], + count: 1, + }, + }); + + const staleResponse = await apply(app, 'surface-a', [finding('finding-b')]); + expect(staleResponse.status).toBe(409); + await expect(staleResponse.json()).resolves.toMatchObject({ + error: { code: 'stale-revision' }, + revisionKey: 'git-commit::next-head-sha', + }); + expect(registry.readFindings('surface-a').count).toBe(1); + }); + + it('isolates findings and targeted events between surfaces for one source', async () => { + const { app, registry } = harness(); + const events: ReviewSurfaceChangeEvent[] = []; + registry.subscribe((event) => events.push(event)); + await register(app, snapshot('surface-a')); + await register(app, snapshot('surface-b')); + + const response = await apply(app, 'surface-a', [finding('finding-a')]); + + expect(response.status).toBe(200); + expect(registry.readFindings('surface-a').count).toBe(1); + expect(registry.readFindings('surface-b').count).toBe(0); + expect( + events.filter((event) => event.action === 'findings-changed'), + ).toEqual([ + expect.objectContaining({ + surfaceId: 'surface-a', + findings: expect.objectContaining({ findingIds: ['finding-a'] }), + }), + ]); + }); + + it('cleans up ephemeral findings on expiry and explicit close', () => { + let now = Date.parse('2026-07-18T00:00:00.000Z'); + const registry = new ReviewSurfaceRegistry({ now: () => now, ttlMs: 100 }); + registries.push(registry); + registry.upsert(snapshot('surface-expiring')); + expect( + registry.applyFindings('surface-expiring', { + revisionKey: 'git-commit::head-sha', + findings: [finding('reusable-id')], + }).ok, + ).toBe(true); + + now += 101; + expect(registry.list()).toEqual([]); + registry.upsert(snapshot('surface-expiring')); + expect( + registry.applyFindings('surface-expiring', { + revisionKey: 'git-commit::head-sha', + findings: [ + finding('reusable-id', { title: 'Content after expiry cleanup' }), + ], + }), + ).toMatchObject({ ok: true, changed: true, count: 1 }); + + registry.upsert(snapshot('surface-closed')); + registry.applyFindings('surface-closed', { + revisionKey: 'git-commit::head-sha', + findings: [finding('closed-id')], + }); + expect(registry.remove('surface-closed')).toBe(true); + registry.upsert(snapshot('surface-closed')); + expect( + registry.applyFindings('surface-closed', { + revisionKey: 'git-commit::head-sha', + findings: [ + finding('closed-id', { title: 'Content after close cleanup' }), + ], + }), + ).toMatchObject({ ok: true, changed: true, count: 1 }); + }); + + it('enforces text and batch limits without publishing large event payloads', async () => { + const { app, registry } = harness(); + const events: ReviewSurfaceChangeEvent[] = []; + registry.subscribe((event) => events.push(event)); + await register(app, snapshot('surface-a')); + const oversizedBatch = Array.from( + { length: neonReviewFindingLimits.maxApplyBatch + 1 }, + (_, index) => finding(`finding-${index}`), + ); + + const batchResponse = await apply(app, 'surface-a', oversizedBatch); + expect(batchResponse.status).toBe(400); + const textResponse = await apply(app, 'surface-a', [ + finding('finding-long', { + title: 'x'.repeat(neonReviewFindingLimits.maxTitleLength + 1), + }), + ]); + expect(textResponse.status).toBe(400); + expect(registry.readFindings('surface-a').count).toBe(0); + + expect( + ( + await apply(app, 'surface-a', [ + finding('finding-bounded', { + explanation: 'private finding explanation', + }), + ]) + ).status, + ).toBe(200); + const event = events.at(-1)!; + expect(event).toMatchObject({ + action: 'findings-changed', + findings: { + action: 'applied', + findingIds: ['finding-bounded'], + count: 1, + }, + }); + expect(JSON.stringify(event)).not.toContain('private finding explanation'); + expect(event.surface).toBeNull(); + }); + + it('caps per-surface retention and bounds bulk-change event ids', () => { + const registry = new ReviewSurfaceRegistry(); + registries.push(registry); + const events: ReviewSurfaceChangeEvent[] = []; + registry.subscribe((event) => events.push(event)); + registry.upsert(snapshot('surface-a')); + + const batchCount = + neonReviewFindingLimits.maxFindingsPerSurface / + neonReviewFindingLimits.maxApplyBatch; + for (let batch = 0; batch < batchCount; batch += 1) { + const findings = Array.from( + { length: neonReviewFindingLimits.maxApplyBatch }, + (_, index) => finding(`finding-${batch}-${index}`), + ); + expect( + registry.applyFindings('surface-a', { + revisionKey: 'git-commit::head-sha', + findings, + }), + ).toMatchObject({ + ok: true, + changed: true, + count: neonReviewFindingLimits.maxApplyBatch, + }); + } + expect(registry.readFindings('surface-a').count).toBe( + neonReviewFindingLimits.maxFindingsPerSurface, + ); + expect( + registry.applyFindings('surface-a', { + revisionKey: 'git-commit::head-sha', + findings: [finding('finding-overflow')], + }), + ).toMatchObject({ + ok: false, + changed: false, + error: { code: 'surface-finding-limit' }, + }); + expect(registry.readFindings('surface-a').count).toBe( + neonReviewFindingLimits.maxFindingsPerSurface, + ); + + expect(registry.clearFindings('surface-a', {})).toMatchObject({ + ok: true, + changed: true, + count: neonReviewFindingLimits.maxFindingsPerSurface, + }); + expect(events.at(-1)).toMatchObject({ + action: 'findings-changed', + findings: { + action: 'cleared', + count: neonReviewFindingLimits.maxFindingsPerSurface, + }, + }); + expect(events.at(-1)?.findings?.findingIds).toHaveLength( + neonReviewFindingLimits.maxEventFindingIds, + ); + }); + + it('makes identical apply and dismiss operations idempotent', async () => { + const { app, registry } = harness(); + const events: ReviewSurfaceChangeEvent[] = []; + registry.subscribe((event) => events.push(event)); + await register(app, snapshot('surface-a')); + + const first = await apply(app, 'surface-a', [finding('finding-a')]); + const second = await apply(app, 'surface-a', [finding('finding-a')]); + expect(first.status).toBe(200); + expect(second.status).toBe(200); + await expect(second.json()).resolves.toMatchObject({ + ok: true, + changed: false, + count: 0, + }); + const conflictingBatch = await apply(app, 'surface-a', [ + finding('finding-b'), + finding('finding-a', { title: 'Conflicting stable id content' }), + ]); + expect(conflictingBatch.status).toBe(409); + await expect(conflictingBatch.json()).resolves.toMatchObject({ + changed: false, + error: { code: 'finding-id-conflict' }, + }); + expect(registry.readFindings('surface-a').count).toBe(1); + + const dismissBody = JSON.stringify({ + findingIds: ['finding-a'], + reason: 'Not actionable.', + }); + const dismiss = () => + app.request( + 'http://localhost/api/review-surfaces/surface-a/findings/dismiss', + { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: dismissBody, + }, + ); + expect((await dismiss()).status).toBe(200); + const duplicateDismiss = await dismiss(); + await expect(duplicateDismiss.json()).resolves.toMatchObject({ + changed: false, + count: 0, + }); + expect( + events.filter((event) => event.action === 'findings-changed'), + ).toHaveLength(2); + expect(registry.readFindings('surface-a')).toMatchObject({ + findings: [{ lifecycle: { state: 'dismissed' } }], + }); + + const clear = () => + app.request( + 'http://localhost/api/review-surfaces/surface-a/findings/clear', + { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ findingIds: ['finding-a'] }), + }, + ); + expect((await clear()).status).toBe(200); + const duplicateClear = await clear(); + await expect(duplicateClear.json()).resolves.toMatchObject({ + changed: false, + count: 0, + }); + expect(registry.readFindings('surface-a').count).toBe(0); + expect( + events.filter((event) => event.action === 'findings-changed'), + ).toHaveLength(3); + }); }); function harness() { @@ -186,7 +497,10 @@ async function register(app: Hono, value: ReviewSurfaceSnapshot) { expect(response.status).toBe(200); } -function snapshot(surfaceId: string): ReviewSurfaceSnapshot { +function snapshot( + surfaceId: string, + revisionId = 'head-sha', +): ReviewSurfaceSnapshot { return { schemaVersion: reviewSurfaceSchemaVersion, surfaceId, @@ -197,7 +511,7 @@ function snapshot(surfaceId: string): ReviewSurfaceSnapshot { title: 'Review surface contract', revision: resolvedReviewRevision({ kind: 'git-commit', - id: 'head-sha', + id: revisionId, }), repository: { repoId: 'repo-1', @@ -231,3 +545,51 @@ function snapshot(surfaceId: string): ReviewSurfaceSnapshot { annotationVisibility: ['threads', 'drafts', 'findings'], }; } + +function finding( + id: string, + overrides: Partial = {}, +): NeonReviewFindingDraft { + return { + schemaVersion: neonReviewFindingSchemaVersion, + id, + sourceId: 'github-pr:example/repo#42', + revisionKey: 'git-commit::head-sha', + file: 'src/app.ts', + anchor: { + kind: 'line-range', + side: 'additions', + startLine: 10, + endLine: 11, + }, + title: 'Check this behavior', + explanation: 'The behavior may not match the intended contract.', + severity: 'major', + confidence: 'high', + suggestedAction: null, + provenance: { + authorRole: 'display-assistant', + model: 'openai/gpt-5.6', + workflowRunId: 'run-1', + }, + ...overrides, + }; +} + +function apply( + app: Hono, + surfaceId: string, + findings: NeonReviewFindingDraft[], +) { + return app.request( + `http://localhost/api/review-surfaces/${surfaceId}/findings/apply`, + { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + revisionKey: 'git-commit::head-sha', + findings, + }), + }, + ); +} diff --git a/src/server/events/event-stream.test.ts b/src/server/events/event-stream.test.ts index 25a6f1ab..69a7797e 100644 --- a/src/server/events/event-stream.test.ts +++ b/src/server/events/event-stream.test.ts @@ -171,6 +171,7 @@ function reviewSurfaceEvent(): ReviewSurfaceChangeEvent { surface: null, navigation: null, acknowledgement: null, + findings: null, reason: 'closed', }; } diff --git a/src/server/routes/review-surfaces.ts b/src/server/routes/review-surfaces.ts index adcbc8fc..e0a61867 100644 --- a/src/server/routes/review-surfaces.ts +++ b/src/server/routes/review-surfaces.ts @@ -1,6 +1,9 @@ import { Hono } from 'hono'; import * as v from 'valibot'; import { + reviewSurfaceFindingsApplySchema, + reviewSurfaceFindingsClearSchema, + reviewSurfaceFindingsDismissSchema, reviewSurfaceNavigationAckInputSchema, reviewSurfaceNavigationRequestSchema, reviewSurfaceRegistry, @@ -49,6 +52,41 @@ export function createReviewSurfaceRoutes( : c.json({ ok: false, message: 'Review surface is not active.' }, 404); }); + routes.get('/review-surfaces/:surfaceId/findings', (c) => { + const result = registry.readFindings(c.req.param('surfaceId')); + return result.ok ? c.json(result) : c.json(result, 404); + }); + + routes.post('/review-surfaces/:surfaceId/findings/apply', async (c) => { + const body = await readJson(c); + const parsed = v.safeParse(reviewSurfaceFindingsApplySchema, body); + if (!parsed.success) return invalidInput(c, parsed.issues); + return findingResult( + c, + registry.applyFindings(c.req.param('surfaceId'), parsed.output), + ); + }); + + routes.post('/review-surfaces/:surfaceId/findings/dismiss', async (c) => { + const body = await readJson(c); + const parsed = v.safeParse(reviewSurfaceFindingsDismissSchema, body); + if (!parsed.success) return invalidInput(c, parsed.issues); + return findingResult( + c, + registry.dismissFindings(c.req.param('surfaceId'), parsed.output), + ); + }); + + routes.post('/review-surfaces/:surfaceId/findings/clear', async (c) => { + const body = await readJson(c); + const parsed = v.safeParse(reviewSurfaceFindingsClearSchema, body); + if (!parsed.success) return invalidInput(c, parsed.issues); + return findingResult( + c, + registry.clearFindings(c.req.param('surfaceId'), parsed.output), + ); + }); + routes.post('/review-surfaces/:surfaceId/navigation', async (c) => { const body = await readJson(c); const parsed = v.safeParse(reviewSurfaceNavigationRequestSchema, body); @@ -106,3 +144,16 @@ function invalidInput( 400, ); } + +function findingResult( + c: { + json: (body: unknown, status?: 200 | 404 | 409) => Response; + }, + result: ReturnType, +) { + if (result.ok) return c.json(result, 200); + return c.json( + result, + result.error?.code === 'surface-not-active' ? 404 : 409, + ); +} From c2aa6103d7b36e02e54cc4787130734e0cf1f7fe Mon Sep 17 00:00:00 2001 From: syn Date: Sat, 18 Jul 2026 10:50:14 -0500 Subject: [PATCH 2/2] Harden review finding lifecycle boundaries --- shared/review-finding.ts | 11 +- shared/review-navigation.ts | 4 +- shared/review-surface.ts | 43 ++- src/agents/display-assistant.ts | 2 +- src/modules/review-surfaces/actions.ts | 40 ++- src/modules/review-surfaces/context.ts | 65 ++++ src/modules/review-surfaces/index.ts | 8 + src/modules/review-surfaces/provenance.ts | 34 ++ src/modules/review-surfaces/registry.ts | 83 ++++- src/modules/review-surfaces/schemas.ts | 47 ++- src/review-surfaces.test.ts | 382 +++++++++++++++++++++- src/server/routes/review-surfaces.ts | 10 +- 12 files changed, 687 insertions(+), 42 deletions(-) create mode 100644 src/modules/review-surfaces/context.ts create mode 100644 src/modules/review-surfaces/provenance.ts diff --git a/shared/review-finding.ts b/shared/review-finding.ts index 06e146ae..383e0904 100644 --- a/shared/review-finding.ts +++ b/shared/review-finding.ts @@ -69,17 +69,26 @@ export type NeonReviewFindingDraft = Omit< provenance: Omit; }; +export type NeonReviewFindingSubmission = Omit< + NeonReviewFindingDraft, + 'provenance' +>; + export type ReviewSurfaceFindingsApplyRequest = { revisionKey: string; - findings: NeonReviewFindingDraft[]; + findings: NeonReviewFindingSubmission[]; }; export type ReviewSurfaceFindingsDismissRequest = { + sourceId: string; + revisionKey: string; findingIds: string[]; reason: string | null; }; export type ReviewSurfaceFindingsClearRequest = { + sourceId: string; + revisionKey: string; findingIds?: string[]; }; diff --git a/shared/review-navigation.ts b/shared/review-navigation.ts index 08483292..5eb76e19 100644 --- a/shared/review-navigation.ts +++ b/shared/review-navigation.ts @@ -1,4 +1,6 @@ -export type ReviewFindingSeverity = 'critical' | 'major' | 'minor' | 'nit'; +import type { NeonReviewFindingSeverity } from './review-finding'; + +export type ReviewFindingSeverity = NeonReviewFindingSeverity; export type ReviewNavigationFile = { path: string; diff --git a/shared/review-surface.ts b/shared/review-surface.ts index 5a3650f4..e100dd9e 100644 --- a/shared/review-surface.ts +++ b/shared/review-surface.ts @@ -1,8 +1,17 @@ import type { ReviewSourceSnapshot } from './review-source'; -import type { ReviewSurfaceFindingChange } from './review-finding'; +import type { + NeonReviewFinding, + ReviewSurfaceFindingChange, +} from './review-finding'; export const reviewSurfaceSchemaVersion = 1 as const; +export const reviewSurfaceContextPageLimits = { + defaultLimit: 25, + maxLimit: 50, + maxOffset: 5_000, +} as const; + export type ReviewSurfaceSelection = { path: string; side: 'additions' | 'deletions'; @@ -32,6 +41,38 @@ export type ActiveReviewSurface = ReviewSurfaceSnapshot & { lastNavigationAck: ReviewSurfaceNavigationAck | null; }; +export type ReviewSurfaceContextPageRequest = { + surfaceId: string; + offset?: number; + limit?: number; +}; + +export type ReviewSurfaceContextWindow = { + items: T[]; + offset: number; + limit: number; + total: number; + nextOffset: number | null; +}; + +export type ReviewSurfaceContextSummary = Omit< + ActiveReviewSurface, + 'source' | 'reviewOrder' +> & { + source: Omit; + counts: { + files: number; + reviewOrder: number; + findings: number; + }; +}; + +export type ReviewSurfaceContextPage = { + files: ReviewSurfaceContextWindow; + reviewOrder: ReviewSurfaceContextWindow; + findings: ReviewSurfaceContextWindow; +}; + export type ReviewSurfaceNavigationTarget = { path: string; focus: boolean; diff --git a/src/agents/display-assistant.ts b/src/agents/display-assistant.ts index 4f1d0b53..0aa100a4 100644 --- a/src/agents/display-assistant.ts +++ b/src/agents/display-assistant.ts @@ -73,7 +73,7 @@ export default defineAgent(({ id }) => { 'For morning briefings, use neondeck_briefing_profile_read, neondeck_briefing_profile_update, neondeck_briefing_run_now, and neondeck_briefing_run_read for exact persisted grounding. Briefing instructions may request any configured MCP source, but do not add a briefing-specific allowlist or auto-approve mutations. For other scheduled work, use neondeck_scheduled_task_instruction_create. Scheduled instructions run as bounded workflows by default; select an explicit agent session target only when the user needs continuity. Use the scheduled-task list, read, pause, resume, and delete actions to operate existing tasks.', 'For autopilot status, use neondeck_autopilot_state_lookup before explaining what Neon is watching, why it did or did not act, which worktrees are prepared, which approvals are pending, and what repo/watch policy allows. Treat this as read-only operator state; do not invent queue entries, diffs, pushes, or workflow outcomes that are not present in the lookup.', 'For prepared autopilot diffs, use neondeck_prepared_diff_list, neondeck_prepared_diff_summary, neondeck_prepared_diff_changed_files, and neondeck_prepared_diff_file_diff for facts. Use neondeck_autopilot_recovery_options before recommending recovery from prepared, blocked, pushed, or failed autopilot states. Use neondeck_autopilot_recovery_run for bounded inspect, retry-after-new-commit, rebase/resync worktree, retry verify, retry push, retry comment, request revision, cleanup worktree, abandon, or manual-follow-up decisions. These recovery actions dispatch to existing prepared-diff, worktree sync/cleanup, and autopilot services, keep the source worktree as the source of truth, and never bypass confirmation, execution, policy, cleanup, or GitHub gates.', - 'For a live diff, use neondeck_review_surfaces_lookup to resolve the intended window and neondeck_review_surface_context_lookup to read its bounded source metadata and ephemeral findings. Use neondeck_review_surface_navigate only for one explicit surface id, keeping focus=false unless the user asked to be shown the target. Apply findings only through neondeck_review_surface_findings_apply with the exact active source and revision, stable ids, concise bounded text, and provenance; dismiss or clear them explicitly. Review-surface findings are process-ephemeral local context only and never create GitHub comments, submit reviews, request prepared-diff revisions, or silently re-anchor across revisions.', + 'For a live diff, use neondeck_review_surfaces_lookup to resolve the intended window and neondeck_review_surface_context_lookup to page through its bounded source metadata and ephemeral findings. Use neondeck_review_surface_navigate only for one explicit surface id, keeping focus=false unless the user asked to be shown the target. Apply findings only through neondeck_review_surface_findings_apply with the exact active source and revision, stable ids, and concise bounded text; Neondeck stamps trusted provenance. Dismiss or clear findings explicitly with the exact active source and revision. Review-surface findings are process-ephemeral local context only and never create GitHub comments, submit reviews, request prepared-diff revisions, or silently re-anchor across revisions.', 'The neondeck_autopilot_prepare_pr_worktree, neondeck_autopilot_fix_pr_review_feedback, neondeck_autopilot_fix_pr_ci_failure, and neondeck_autopilot_push_pr_autofix actions are workflow-only and refuse in interactive chat. Autonomous watcher workflows use them under a Flue run id and remain governed by the configured per-PR mode.', 'For PR event facts and watermarks, use neondeck_github_pr_event_state_get, neondeck_pr_review_comments_lookup, neondeck_pr_requested_changes_lookup, neondeck_pr_branch_permissions_lookup, neondeck_pr_watch_event_state_refresh, and neondeck_pr_watch_event_watermarks_list. These GitHub fact collectors and app-state watermarks do not prepare fixes or push. Use neondeck_pr_comment only when the intended PR comment text is explicit and grounded in deterministic facts; prefer neondeck_autopilot_comment_pr_autofix_result for autonomous prepared-diff result comments because it also records the workflow summary audit.', 'For quick deterministic facts, prefer the neondeck_*_lookup tools. Use actions when you need a durable command, mutation, scheduler tick, or persisted workflow summary.', diff --git a/src/modules/review-surfaces/actions.ts b/src/modules/review-surfaces/actions.ts index 20f48b6e..52342df0 100644 --- a/src/modules/review-surfaces/actions.ts +++ b/src/modules/review-surfaces/actions.ts @@ -1,13 +1,18 @@ import { defineAction, defineTool } from '@flue/runtime'; import * as v from 'valibot'; import { currentFlueExecutionContext } from '../flue'; +import { createReviewSurfaceContextPage } from './context'; +import { + flueFindingProvenance, + stampReviewFindingSubmissions, +} from './provenance'; import { reviewSurfaceRegistry } from './registry'; import { reviewSurfaceActionOutputSchema, + reviewSurfaceContextInputSchema, reviewSurfaceFindingsApplyActionSchema, reviewSurfaceFindingsClearActionSchema, reviewSurfaceFindingsDismissActionSchema, - reviewSurfaceIdInputSchema, reviewSurfaceNavigateInputSchema, } from './schemas'; @@ -43,8 +48,8 @@ export const reviewSurfacesLookupTool = defineTool({ export const reviewSurfaceContextLookupTool = defineTool({ name: 'neondeck_review_surface_context_lookup', description: - 'Read one active review surface and its bounded process-ephemeral Neon findings. This does not load patch bodies.', - input: reviewSurfaceIdInputSchema, + 'Read a paged summary of one active review surface and its process-ephemeral Neon findings. Files, review order, and findings share an offset and bounded page limit; patch bodies are never loaded.', + input: reviewSurfaceContextInputSchema, output: lookupOutputSchema, async run({ input }) { const surface = reviewSurfaceRegistry.read(input.surfaceId); @@ -55,10 +60,12 @@ export const reviewSurfaceContextLookupTool = defineTool({ surfaceId: input.surfaceId, }; } - return { - ...reviewSurfaceRegistry.readFindings(input.surfaceId), + const findingResult = reviewSurfaceRegistry.readFindings(input.surfaceId); + return createReviewSurfaceContextPage( surface, - }; + findingResult.findings ?? [], + input, + ); }, }); @@ -101,16 +108,13 @@ export const reviewSurfaceFindingsApplyAction = defineAction({ input: reviewSurfaceFindingsApplyActionSchema, output: reviewSurfaceActionOutputSchema, async run({ input }) { - const workflowRunId = currentFlueExecutionContext()?.runId ?? null; + const context = currentFlueExecutionContext(); return reviewSurfaceRegistry.applyFindings(input.surfaceId, { revisionKey: input.revisionKey, - findings: input.findings.map((finding) => ({ - ...finding, - provenance: { - ...finding.provenance, - workflowRunId: workflowRunId ?? finding.provenance.workflowRunId, - }, - })), + findings: stampReviewFindingSubmissions( + input.findings, + flueFindingProvenance(context), + ), }); }, }); @@ -118,11 +122,13 @@ export const reviewSurfaceFindingsApplyAction = defineAction({ export const reviewSurfaceFindingsDismissAction = defineAction({ name: 'neondeck_review_surface_findings_dismiss', description: - 'Explicitly dismiss selected process-ephemeral Neon findings on one active review surface.', + 'Explicitly dismiss selected process-ephemeral Neon findings on one active review surface when the expected source and revision are still mounted.', input: reviewSurfaceFindingsDismissActionSchema, output: reviewSurfaceActionOutputSchema, async run({ input }) { return reviewSurfaceRegistry.dismissFindings(input.surfaceId, { + sourceId: input.sourceId, + revisionKey: input.revisionKey, findingIds: input.findingIds, reason: input.reason, }); @@ -132,11 +138,13 @@ export const reviewSurfaceFindingsDismissAction = defineAction({ export const reviewSurfaceFindingsClearAction = defineAction({ name: 'neondeck_review_surface_findings_clear', description: - 'Explicitly remove selected or all process-ephemeral Neon findings from one active review surface.', + 'Explicitly remove selected or all process-ephemeral Neon findings from one active review surface when the expected source and revision are still mounted.', input: reviewSurfaceFindingsClearActionSchema, output: reviewSurfaceActionOutputSchema, async run({ input }) { return reviewSurfaceRegistry.clearFindings(input.surfaceId, { + sourceId: input.sourceId, + revisionKey: input.revisionKey, findingIds: input.findingIds, }); }, diff --git a/src/modules/review-surfaces/context.ts b/src/modules/review-surfaces/context.ts new file mode 100644 index 00000000..37e0daf0 --- /dev/null +++ b/src/modules/review-surfaces/context.ts @@ -0,0 +1,65 @@ +import type { NeonReviewFinding } from '../../../shared/review-finding'; +import { + reviewSurfaceContextPageLimits, + type ActiveReviewSurface, + type ReviewSurfaceContextPage, + type ReviewSurfaceContextPageRequest, + type ReviewSurfaceContextWindow, +} from '../../../shared/review-surface'; + +export function createReviewSurfaceContextPage( + surface: ActiveReviewSurface, + findings: NeonReviewFinding[], + request: Omit = {}, +) { + const offset = clampInteger( + request.offset ?? 0, + 0, + reviewSurfaceContextPageLimits.maxOffset, + ); + const limit = clampInteger( + request.limit ?? reviewSurfaceContextPageLimits.defaultLimit, + 1, + reviewSurfaceContextPageLimits.maxLimit, + ); + const { files, ...source } = surface.source; + const { reviewOrder, source: _source, ...surfaceSummary } = surface; + const page: ReviewSurfaceContextPage = { + files: window(files, offset, limit), + reviewOrder: window(reviewOrder, offset, limit), + findings: window(findings, offset, limit), + }; + return { + ok: true as const, + summary: { + ...surfaceSummary, + source, + counts: { + files: files.length, + reviewOrder: reviewOrder.length, + findings: findings.length, + }, + }, + page, + }; +} + +function window( + values: readonly T[], + offset: number, + limit: number, +): ReviewSurfaceContextWindow { + const items = values.slice(offset, offset + limit); + return { + items, + offset, + limit, + total: values.length, + nextOffset: offset + items.length < values.length ? offset + limit : null, + }; +} + +function clampInteger(value: number, min: number, max: number) { + if (!Number.isFinite(value)) return min; + return Math.min(max, Math.max(min, Math.trunc(value))); +} diff --git a/src/modules/review-surfaces/index.ts b/src/modules/review-surfaces/index.ts index 4088b19c..dc4f3a7a 100644 --- a/src/modules/review-surfaces/index.ts +++ b/src/modules/review-surfaces/index.ts @@ -10,8 +10,10 @@ export type { } from './registry'; export { neonReviewFindingDraftSchema, + neonReviewFindingSubmissionSchema, neonReviewFindingSchema, reviewSurfaceActionOutputSchema, + reviewSurfaceContextInputSchema, reviewSurfaceFindingsApplyActionSchema, reviewSurfaceFindingsApplySchema, reviewSurfaceFindingsClearActionSchema, @@ -34,3 +36,9 @@ export { reviewSurfaceNavigateAction, reviewSurfacesLookupTool, } from './actions'; +export { createReviewSurfaceContextPage } from './context'; +export { + flueFindingProvenance, + localApiFindingProvenance, + stampReviewFindingSubmissions, +} from './provenance'; diff --git a/src/modules/review-surfaces/provenance.ts b/src/modules/review-surfaces/provenance.ts new file mode 100644 index 00000000..da3486bb --- /dev/null +++ b/src/modules/review-surfaces/provenance.ts @@ -0,0 +1,34 @@ +import type { FlueExecutionContext } from '@flue/runtime'; +import type { + NeonReviewFindingDraft, + NeonReviewFindingProvenance, + NeonReviewFindingSubmission, +} from '../../../shared/review-finding'; + +type FindingProvenanceStamp = Omit; + +export const localApiFindingProvenance = { + authorRole: 'local-api', + model: null, + workflowRunId: null, +} satisfies FindingProvenanceStamp; + +export function flueFindingProvenance( + context: FlueExecutionContext | undefined, +): FindingProvenanceStamp { + return { + authorRole: context?.agentName ?? 'flue', + model: null, + workflowRunId: context?.runId ?? null, + }; +} + +export function stampReviewFindingSubmissions( + findings: readonly NeonReviewFindingSubmission[], + provenance: FindingProvenanceStamp, +): NeonReviewFindingDraft[] { + return findings.map((finding) => ({ + ...finding, + provenance: { ...provenance }, + })); +} diff --git a/src/modules/review-surfaces/registry.ts b/src/modules/review-surfaces/registry.ts index 90a33fb3..2c7fdd35 100644 --- a/src/modules/review-surfaces/registry.ts +++ b/src/modules/review-surfaces/registry.ts @@ -4,7 +4,6 @@ import { type NeonReviewFinding, type NeonReviewFindingDraft, type ReviewSurfaceFindingChange, - type ReviewSurfaceFindingsApplyRequest, type ReviewSurfaceFindingsClearRequest, type ReviewSurfaceFindingsDismissRequest, } from '../../../shared/review-finding'; @@ -28,6 +27,11 @@ type ReviewSurfaceRegistryOptions = { ttlMs?: number; }; +type TrustedReviewSurfaceFindingsApplyRequest = { + revisionKey: string; + findings: NeonReviewFindingDraft[]; +}; + export type ReviewSurfaceFindingErrorCode = | 'surface-not-active' | 'revision-unavailable' @@ -139,7 +143,7 @@ export class ReviewSurfaceRegistry { applyFindings( surfaceId: string, - input: ReviewSurfaceFindingsApplyRequest, + input: TrustedReviewSurfaceFindingsApplyRequest, ): ReviewSurfaceFindingResult { const surface = this.read(surfaceId); if (!surface) @@ -188,20 +192,31 @@ export class ReviewSurfaceRegistry { } const existing = this.findings.get(surfaceId) ?? new Map(); + const findingsToApply: NeonReviewFindingDraft[] = []; + let addedFindingCount = 0; for (const finding of input.findings) { const current = existing.get(finding.id); - if (current && !sameFindingDraft(current, finding)) { + if (!current) { + findingsToApply.push(finding); + addedFindingCount += 1; + continue; + } + if (sameFindingScope(current, finding)) { + if (sameFindingDraft(current, finding)) continue; + return this.findingError(surfaceId, 'apply', 'finding-id-conflict', { + revisionKey: currentRevisionKey, + }); + } + if (current.lifecycle.state === 'active') { return this.findingError(surfaceId, 'apply', 'finding-id-conflict', { revisionKey: currentRevisionKey, }); } + findingsToApply.push(finding); } - const newFindings = input.findings.filter( - (finding) => !existing.has(finding.id), - ); if ( - existing.size + newFindings.length > + existing.size + addedFindingCount > neonReviewFindingLimits.maxFindingsPerSurface ) { return this.findingError(surfaceId, 'apply', 'surface-finding-limit', { @@ -209,7 +224,7 @@ export class ReviewSurfaceRegistry { }); } - if (newFindings.length === 0) { + if (findingsToApply.length === 0) { return { ok: true, action: 'apply', @@ -224,7 +239,7 @@ export class ReviewSurfaceRegistry { } const changedAt = this.timestamp(); - const applied = newFindings.map((finding) => + const applied = findingsToApply.map((finding) => materializeFinding(surfaceId, finding, changedAt), ); const next = new Map(existing); @@ -256,6 +271,13 @@ export class ReviewSurfaceRegistry { const surface = this.read(surfaceId); if (!surface) return this.findingError(surfaceId, 'dismiss', 'surface-not-active'); + const scopeError = this.findingMutationScopeError( + surface, + 'dismiss', + input.sourceId, + input.revisionKey, + ); + if (scopeError) return scopeError; if ( input.findingIds.length === 0 || input.findingIds.length > neonReviewFindingLimits.maxApplyBatch @@ -272,6 +294,12 @@ export class ReviewSurfaceRegistry { for (const findingId of input.findingIds) { const finding = next.get(findingId); if (!finding || finding.lifecycle.state === 'dismissed') continue; + if ( + finding.lifecycle.state !== 'active' && + finding.lifecycle.state !== 'stale' + ) { + continue; + } next.set(findingId, { ...finding, lifecycle: { @@ -310,6 +338,13 @@ export class ReviewSurfaceRegistry { const surface = this.read(surfaceId); if (!surface) return this.findingError(surfaceId, 'clear', 'surface-not-active'); + const scopeError = this.findingMutationScopeError( + surface, + 'clear', + input.sourceId, + input.revisionKey, + ); + if (scopeError) return scopeError; if ( input.findingIds && (input.findingIds.length === 0 || @@ -576,6 +611,26 @@ export class ReviewSurfaceRegistry { error: { code, message }, }; } + + private findingMutationScopeError( + surface: ActiveReviewSurface, + action: string, + sourceId: string, + revisionKey: string, + ) { + const currentRevisionKey = reviewRevisionKey(surface.source.revision); + if (sourceId !== surface.source.id) { + return this.findingError(surface.surfaceId, action, 'source-mismatch', { + revisionKey: currentRevisionKey, + }); + } + if (revisionKey !== currentRevisionKey) { + return this.findingError(surface.surfaceId, action, 'stale-revision', { + revisionKey: currentRevisionKey, + }); + } + return null; + } } function materializeFinding( @@ -617,6 +672,16 @@ function sameFindingDraft( ); } +function sameFindingScope( + finding: NeonReviewFinding, + draft: NeonReviewFindingDraft, +) { + return ( + finding.sourceId === draft.sourceId && + finding.revisionKey === draft.revisionKey + ); +} + function findingErrorMessage(code: ReviewSurfaceFindingErrorCode) { switch (code) { case 'surface-not-active': diff --git a/src/modules/review-surfaces/schemas.ts b/src/modules/review-surfaces/schemas.ts index 098ca26d..c62372fd 100644 --- a/src/modules/review-surfaces/schemas.ts +++ b/src/modules/review-surfaces/schemas.ts @@ -4,6 +4,7 @@ import { neonReviewFindingSchemaVersion, type NeonReviewFinding, type NeonReviewFindingDraft, + type NeonReviewFindingSubmission, type ReviewSurfaceFindingsApplyRequest, type ReviewSurfaceFindingsClearRequest, type ReviewSurfaceFindingsDismissRequest, @@ -13,7 +14,9 @@ import { type ReviewSourceSnapshot, } from '../../../shared/review-source'; import { + reviewSurfaceContextPageLimits, reviewSurfaceSchemaVersion, + type ReviewSurfaceContextPageRequest, type ReviewSurfaceNavigationRequest, type ReviewSurfaceSnapshot, } from '../../../shared/review-surface'; @@ -180,7 +183,7 @@ const findingProvenanceInputSchema = v.object({ workflowRunId: nullableIdentifierSchema, }); -const neonReviewFindingDraftEntries = { +const neonReviewFindingSubmissionEntries = { schemaVersion: v.literal(neonReviewFindingSchemaVersion), id: identifierSchema, sourceId: identifierSchema, @@ -206,15 +209,20 @@ const neonReviewFindingDraftEntries = { v.maxLength(neonReviewFindingLimits.maxSuggestedActionLength), ), ), - provenance: findingProvenanceInputSchema, }; +export const neonReviewFindingSubmissionSchema: v.GenericSchema = + v.object(neonReviewFindingSubmissionEntries); + export const neonReviewFindingDraftSchema: v.GenericSchema = - v.object(neonReviewFindingDraftEntries); + v.object({ + ...neonReviewFindingSubmissionEntries, + provenance: findingProvenanceInputSchema, + }); export const neonReviewFindingSchema: v.GenericSchema = v.object({ - ...neonReviewFindingDraftEntries, + ...neonReviewFindingSubmissionEntries, surfaceId: surfaceIdSchema, provenance: v.object({ ...findingProvenanceInputSchema.entries, @@ -240,7 +248,7 @@ export const neonReviewFindingSchema: v.GenericSchema = }); const findingBatchSchema = v.pipe( - v.array(neonReviewFindingDraftSchema), + v.array(neonReviewFindingSubmissionSchema), v.minLength(1), v.maxLength(neonReviewFindingLimits.maxApplyBatch), v.check( @@ -258,6 +266,8 @@ export const reviewSurfaceFindingsApplySchema: v.GenericSchema = v.object({ + sourceId: identifierSchema, + revisionKey: revisionKeySchema, findingIds: findingIdListSchema, reason: v.nullable( v.pipe( @@ -270,6 +280,8 @@ export const reviewSurfaceFindingsDismissSchema: v.GenericSchema = v.object({ + sourceId: identifierSchema, + revisionKey: revisionKeySchema, findingIds: v.optional(findingIdListSchema), }); @@ -277,6 +289,27 @@ export const reviewSurfaceIdInputSchema = v.object({ surfaceId: surfaceIdSchema, }); +export const reviewSurfaceContextInputSchema: v.GenericSchema = + v.object({ + surfaceId: surfaceIdSchema, + offset: v.optional( + v.pipe( + v.number(), + v.integer(), + v.minValue(0), + v.maxValue(reviewSurfaceContextPageLimits.maxOffset), + ), + ), + limit: v.optional( + v.pipe( + v.number(), + v.integer(), + v.minValue(1), + v.maxValue(reviewSurfaceContextPageLimits.maxLimit), + ), + ), + }); + export const reviewSurfaceNavigateInputSchema = v.object({ surfaceId: surfaceIdSchema, revisionKey: v.nullable(revisionKeySchema), @@ -291,6 +324,8 @@ export const reviewSurfaceFindingsApplyActionSchema = v.object({ export const reviewSurfaceFindingsDismissActionSchema = v.object({ surfaceId: surfaceIdSchema, + sourceId: identifierSchema, + revisionKey: revisionKeySchema, findingIds: findingIdListSchema, reason: v.nullable( v.pipe( @@ -303,6 +338,8 @@ export const reviewSurfaceFindingsDismissActionSchema = v.object({ export const reviewSurfaceFindingsClearActionSchema = v.object({ surfaceId: surfaceIdSchema, + sourceId: identifierSchema, + revisionKey: revisionKeySchema, findingIds: v.optional(findingIdListSchema), }); diff --git a/src/review-surfaces.test.ts b/src/review-surfaces.test.ts index c8982427..38e85409 100644 --- a/src/review-surfaces.test.ts +++ b/src/review-surfaces.test.ts @@ -14,7 +14,13 @@ import { type ReviewSurfaceChangeEvent, type ReviewSurfaceSnapshot, } from '../shared/review-surface'; -import { ReviewSurfaceRegistry } from './modules/review-surfaces'; +import { + createReviewSurfaceContextPage, + reviewSurfaceFindingsApplyAction, + reviewSurfaceRegistry, + ReviewSurfaceRegistry, +} from './modules/review-surfaces'; +import { runWithFlueExecutionContextForTests } from './modules/flue/execution-context'; import { createReviewSurfaceRoutes } from './server/routes/review-surfaces'; const registries: ReviewSurfaceRegistry[] = []; @@ -227,7 +233,6 @@ describe('review surface registry', () => { count: 1, }, }); - const staleResponse = await apply(app, 'surface-a', [finding('finding-b')]); expect(staleResponse.status).toBe(409); await expect(staleResponse.json()).resolves.toMatchObject({ @@ -237,6 +242,104 @@ describe('review surface registry', () => { expect(registry.readFindings('surface-a').count).toBe(1); }); + it('replaces a non-active stable id after the surface source and revision change', async () => { + const { app, registry } = harness(); + const events: ReviewSurfaceChangeEvent[] = []; + registry.subscribe((event) => events.push(event)); + await register(app, snapshot('surface-a')); + expect((await apply(app, 'surface-a', [finding('finding-a')])).status).toBe( + 200, + ); + + const nextSourceId = 'github-pr:example/other#42'; + const nextRevisionKey = 'git-commit::next-head-sha'; + await register(app, snapshot('surface-a', 'next-head-sha', nextSourceId)); + const response = await apply( + app, + 'surface-a', + [ + finding('finding-a', { + sourceId: nextSourceId, + revisionKey: nextRevisionKey, + title: 'Current revision finding', + }), + ], + nextRevisionKey, + ); + + expect(response.status).toBe(200); + await expect(response.json()).resolves.toMatchObject({ + ok: true, + changed: true, + findingIds: ['finding-a'], + count: 1, + }); + expect(registry.readFindings('surface-a')).toMatchObject({ + count: 1, + findings: [ + { + id: 'finding-a', + sourceId: nextSourceId, + revisionKey: nextRevisionKey, + title: 'Current revision finding', + lifecycle: { state: 'active' }, + }, + ], + }); + expect(events.at(-1)).toMatchObject({ + action: 'findings-changed', + surfaceId: 'surface-a', + findings: { + action: 'applied', + findingIds: ['finding-a'], + count: 1, + }, + }); + expect( + events.filter((event) => event.findings?.action === 'applied'), + ).toHaveLength(2); + }); + + it('replaces a dismissed stable id after the surface revision changes', () => { + const registry = new ReviewSurfaceRegistry(); + registries.push(registry); + registry.upsert(snapshot('surface-a')); + registry.applyFindings('surface-a', { + revisionKey: 'git-commit::head-sha', + findings: [finding('finding-a')], + }); + registry.dismissFindings('surface-a', { + sourceId: 'github-pr:example/repo#42', + revisionKey: 'git-commit::head-sha', + findingIds: ['finding-a'], + reason: 'Dismissed on the prior revision.', + }); + registry.upsert(snapshot('surface-a', 'next-head-sha')); + + expect( + registry.applyFindings('surface-a', { + revisionKey: 'git-commit::next-head-sha', + findings: [ + finding('finding-a', { + revisionKey: 'git-commit::next-head-sha', + title: 'Valid finding on the current revision', + }), + ], + }), + ).toMatchObject({ ok: true, changed: true, count: 1 }); + expect(registry.readFindings('surface-a')).toMatchObject({ + count: 1, + findings: [ + { + id: 'finding-a', + revisionKey: 'git-commit::next-head-sha', + title: 'Valid finding on the current revision', + lifecycle: { state: 'active' }, + }, + ], + }); + }); + it('isolates findings and targeted events between surfaces for one source', async () => { const { app, registry } = harness(); const events: ReviewSurfaceChangeEvent[] = []; @@ -342,6 +445,226 @@ describe('review surface registry', () => { expect(event.surface).toBeNull(); }); + it('server-stamps local API provenance instead of trusting caller fields', async () => { + const { app, registry } = harness(); + await register(app, snapshot('surface-a')); + + const response = await apply(app, 'surface-a', [ + finding('finding-spoofed', { + provenance: { + authorRole: 'untrusted-caller', + model: 'spoofed-model', + workflowRunId: 'spoofed-run', + }, + }), + ]); + + expect(response.status).toBe(200); + expect(registry.readFindings('surface-a')).toMatchObject({ + findings: [ + { + id: 'finding-spoofed', + provenance: { + authorRole: 'local-api', + model: null, + workflowRunId: null, + }, + }, + ], + }); + expect( + registry.readFindings('surface-a').findings?.[0]?.provenance.createdAt, + ).toMatch(/^\d{4}-\d{2}-\d{2}T/); + }); + + it('server-stamps Flue provenance from the current execution context', async () => { + const surfaceId = 'surface-flue-provenance'; + reviewSurfaceRegistry.remove(surfaceId); + reviewSurfaceRegistry.upsert(snapshot(surfaceId)); + try { + const result = await runWithFlueExecutionContextForTests( + { agentName: 'display-assistant', runId: 'trusted-run' }, + () => + reviewSurfaceFindingsApplyAction.run({ + input: { + surfaceId, + revisionKey: 'git-commit::head-sha', + findings: [ + finding('finding-flue-spoofed', { + provenance: { + authorRole: 'untrusted-caller', + model: 'spoofed-model', + workflowRunId: 'spoofed-run', + }, + }), + ], + }, + } as never), + ); + + expect(result).toMatchObject({ ok: true, changed: true }); + expect(reviewSurfaceRegistry.readFindings(surfaceId)).toMatchObject({ + findings: [ + { + provenance: { + authorRole: 'display-assistant', + model: null, + workflowRunId: 'trusted-run', + }, + }, + ], + }); + } finally { + reviewSurfaceRegistry.remove(surfaceId); + } + }); + + it('pages model-facing context with bounded windows and totals', () => { + const registry = new ReviewSurfaceRegistry(); + registries.push(registry); + const base = snapshot('surface-a'); + const files = Array.from({ length: 60 }, (_, index) => ({ + ...base.source.files[0]!, + path: `src/file-${index}.ts`, + })); + const surface = registry.upsert({ + ...base, + source: { ...base.source, files }, + activePath: files[0]!.path, + reviewOrder: files.map((file) => file.path), + }); + const findings = Array.from({ length: 55 }, (_, index) => + finding(`finding-${index}`, { file: files[index]!.path }), + ); + registry.applyFindings('surface-a', { + revisionKey: 'git-commit::head-sha', + findings: findings.slice(0, 50), + }); + registry.applyFindings('surface-a', { + revisionKey: 'git-commit::head-sha', + findings: findings.slice(50), + }); + const stored = registry.readFindings('surface-a').findings ?? []; + + const defaultPage = createReviewSurfaceContextPage(surface, stored); + expect(defaultPage.summary.counts).toEqual({ + files: 60, + reviewOrder: 60, + findings: 55, + }); + expect(defaultPage.summary.source).not.toHaveProperty('files'); + expect(defaultPage.summary).not.toHaveProperty('reviewOrder'); + expect(defaultPage.page.files).toMatchObject({ + offset: 0, + limit: 25, + total: 60, + nextOffset: 25, + }); + expect(defaultPage.page.files.items).toHaveLength(25); + expect(defaultPage.page.reviewOrder.items).toHaveLength(25); + expect(defaultPage.page.findings.items).toHaveLength(25); + + const maximumPage = createReviewSurfaceContextPage(surface, stored, { + limit: 500, + }); + expect(maximumPage.page.files).toMatchObject({ + limit: 50, + total: 60, + nextOffset: 50, + }); + expect(maximumPage.page.files.items).toHaveLength(50); + expect(maximumPage.page.reviewOrder.items).toHaveLength(50); + expect(maximumPage.page.findings.items).toHaveLength(50); + }); + + it('rejects delayed dismiss and clear requests after the mounted revision changes', () => { + const registry = new ReviewSurfaceRegistry(); + registries.push(registry); + registry.upsert(snapshot('surface-a')); + registry.applyFindings('surface-a', { + revisionKey: 'git-commit::head-sha', + findings: [finding('finding-a')], + }); + const delayedDismiss = { + sourceId: 'github-pr:example/repo#42', + revisionKey: 'git-commit::head-sha', + findingIds: ['finding-a'], + reason: 'Delayed dismissal.', + }; + const delayedClear = { + sourceId: 'github-pr:example/repo#42', + revisionKey: 'git-commit::head-sha', + }; + + registry.upsert(snapshot('surface-a', 'next-head-sha')); + registry.applyFindings('surface-a', { + revisionKey: 'git-commit::next-head-sha', + findings: [ + finding('finding-a', { + revisionKey: 'git-commit::next-head-sha', + title: 'Current revision content', + }), + ], + }); + + expect(registry.dismissFindings('surface-a', delayedDismiss)).toMatchObject( + { + ok: false, + changed: false, + error: { code: 'stale-revision' }, + }, + ); + expect(registry.clearFindings('surface-a', delayedClear)).toMatchObject({ + ok: false, + changed: false, + error: { code: 'stale-revision' }, + }); + expect( + registry.clearFindings('surface-a', { + sourceId: 'github-pr:example/other#42', + revisionKey: 'git-commit::next-head-sha', + }), + ).toMatchObject({ + ok: false, + changed: false, + error: { code: 'source-mismatch' }, + }); + expect(registry.readFindings('surface-a')).toMatchObject({ + count: 1, + findings: [ + { + id: 'finding-a', + revisionKey: 'git-commit::next-head-sha', + title: 'Current revision content', + lifecycle: { state: 'active' }, + }, + ], + }); + }); + + it('allows a current-revision command to dismiss a stale finding', () => { + const registry = new ReviewSurfaceRegistry(); + registries.push(registry); + registry.upsert(snapshot('surface-a')); + registry.applyFindings('surface-a', { + revisionKey: 'git-commit::head-sha', + findings: [finding('finding-a')], + }); + registry.upsert(snapshot('surface-a', 'next-head-sha')); + + expect( + registry.dismissFindings('surface-a', { + sourceId: 'github-pr:example/repo#42', + revisionKey: 'git-commit::next-head-sha', + findingIds: ['finding-a'], + reason: 'Acknowledged stale finding.', + }), + ).toMatchObject({ ok: true, changed: true, findingIds: ['finding-a'] }); + expect(registry.readFindings('surface-a')).toMatchObject({ + findings: [{ lifecycle: { state: 'dismissed' } }], + }); + }); + it('caps per-surface retention and bounds bulk-change event ids', () => { const registry = new ReviewSurfaceRegistry(); registries.push(registry); @@ -385,7 +708,41 @@ describe('review surface registry', () => { neonReviewFindingLimits.maxFindingsPerSurface, ); - expect(registry.clearFindings('surface-a', {})).toMatchObject({ + registry.upsert(snapshot('surface-a', 'next-head-sha')); + expect( + registry.applyFindings('surface-a', { + revisionKey: 'git-commit::next-head-sha', + findings: [ + finding('finding-0-0', { + revisionKey: 'git-commit::next-head-sha', + title: 'Replacement at the surface cap', + }), + ], + }), + ).toMatchObject({ ok: true, changed: true, count: 1 }); + expect(registry.readFindings('surface-a').count).toBe( + neonReviewFindingLimits.maxFindingsPerSurface, + ); + expect( + registry.applyFindings('surface-a', { + revisionKey: 'git-commit::next-head-sha', + findings: [ + finding('finding-current-overflow', { + revisionKey: 'git-commit::next-head-sha', + }), + ], + }), + ).toMatchObject({ + ok: false, + error: { code: 'surface-finding-limit' }, + }); + + expect( + registry.clearFindings('surface-a', { + sourceId: 'github-pr:example/repo#42', + revisionKey: 'git-commit::next-head-sha', + }), + ).toMatchObject({ ok: true, changed: true, count: neonReviewFindingLimits.maxFindingsPerSurface, @@ -402,7 +759,7 @@ describe('review surface registry', () => { ); }); - it('makes identical apply and dismiss operations idempotent', async () => { + it('keeps identical applies idempotent and rejects same-revision conflicts atomically', async () => { const { app, registry } = harness(); const events: ReviewSurfaceChangeEvent[] = []; registry.subscribe((event) => events.push(event)); @@ -427,8 +784,13 @@ describe('review surface registry', () => { error: { code: 'finding-id-conflict' }, }); expect(registry.readFindings('surface-a').count).toBe(1); + expect( + events.filter((event) => event.action === 'findings-changed'), + ).toHaveLength(1); const dismissBody = JSON.stringify({ + sourceId: 'github-pr:example/repo#42', + revisionKey: 'git-commit::head-sha', findingIds: ['finding-a'], reason: 'Not actionable.', }); @@ -460,7 +822,11 @@ describe('review surface registry', () => { { method: 'POST', headers: { 'content-type': 'application/json' }, - body: JSON.stringify({ findingIds: ['finding-a'] }), + body: JSON.stringify({ + sourceId: 'github-pr:example/repo#42', + revisionKey: 'git-commit::head-sha', + findingIds: ['finding-a'], + }), }, ); expect((await clear()).status).toBe(200); @@ -500,13 +866,14 @@ async function register(app: Hono, value: ReviewSurfaceSnapshot) { function snapshot( surfaceId: string, revisionId = 'head-sha', + sourceId = 'github-pr:example/repo#42', ): ReviewSurfaceSnapshot { return { schemaVersion: reviewSurfaceSchemaVersion, surfaceId, source: { schemaVersion: reviewSourceSchemaVersion, - id: 'github-pr:example/repo#42', + id: sourceId, kind: 'github-pr', title: 'Review surface contract', revision: resolvedReviewRevision({ @@ -580,6 +947,7 @@ function apply( app: Hono, surfaceId: string, findings: NeonReviewFindingDraft[], + revisionKey = 'git-commit::head-sha', ) { return app.request( `http://localhost/api/review-surfaces/${surfaceId}/findings/apply`, @@ -587,7 +955,7 @@ function apply( method: 'POST', headers: { 'content-type': 'application/json' }, body: JSON.stringify({ - revisionKey: 'git-commit::head-sha', + revisionKey, findings, }), }, diff --git a/src/server/routes/review-surfaces.ts b/src/server/routes/review-surfaces.ts index e0a61867..ff7e8a84 100644 --- a/src/server/routes/review-surfaces.ts +++ b/src/server/routes/review-surfaces.ts @@ -4,10 +4,12 @@ import { reviewSurfaceFindingsApplySchema, reviewSurfaceFindingsClearSchema, reviewSurfaceFindingsDismissSchema, + localApiFindingProvenance, reviewSurfaceNavigationAckInputSchema, reviewSurfaceNavigationRequestSchema, reviewSurfaceRegistry, reviewSurfaceSnapshotSchema, + stampReviewFindingSubmissions, type ReviewSurfaceRegistry, } from '../../modules/review-surfaces'; @@ -63,7 +65,13 @@ export function createReviewSurfaceRoutes( if (!parsed.success) return invalidInput(c, parsed.issues); return findingResult( c, - registry.applyFindings(c.req.param('surfaceId'), parsed.output), + registry.applyFindings(c.req.param('surfaceId'), { + revisionKey: parsed.output.revisionKey, + findings: stampReviewFindingSubmissions( + parsed.output.findings, + localApiFindingProvenance, + ), + }), ); });