feat: folder-scoped review instructions for the deployment ai review (#12037)

* feat: add ai review to deployment requests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: bound ai review fetches and surface unreadable items Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat: add fix in ai session and model picker to deployment ai review Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat: add workspace instructions for deployment ai review Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: read review instructions from the parent and bind ai review to the fork Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: pin the fix-all session preview to the fork's compare page Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: show the protection-rule bypass toggle inline Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: make the bypass toggle small and red Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor: drop workspace-settings instructions from deployment ai review Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat: folder-scoped review instructions for the deployment ai review Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat: review the selected items with ai on compare and deploy Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: keep reviewing when review instructions cannot be listed Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: say the review instruction floor is a prompt request, not enforcement Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: mark an ai review out of date when its items change Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: update ee-repo-ref to 77703cf32e58964906d87ee83ddda44b5b8d1b7e This commit updates the EE repository reference after PR #926 was merged in windmill-ee-private. Previous ee-repo-ref: e30ab9e69d1854fcaed5620c934ace32acb0a729 New ee-repo-ref: 77703cf32e58964906d87ee83ddda44b5b8d1b7e Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: windmill-internal-app[bot] <windmill-internal-app[bot]@users.noreply.github.com>
2026-10-30 08:16:49 +00:00 · 2026-10-28 01:25:07 +01:00
parent 210a235be6
commit 62fbd508c8
7 changed files with 180 additions and 10 deletions
@@ -0,0 +1,6 @@
+-- `created_by` tells this migration's row from one a user created by hand; a hub
+-- sync updates in place and leaves it alone.
+DELETE FROM resource_type
+WHERE workspace_id = 'admins'
+  AND name = 'ai_review_instruction'
+  AND created_by = 'system';
@@ -0,0 +1,15 @@
+-- Review instructions for the AI review of fork deployment requests: a markdown
+-- file resource whose `value.content` applies to every changed item under the
+-- directory it lives in. Read from the parent workspace, the one the changes merge into.
+-- Seeded under 'admins' so every workspace sees it before the next hub sync.
+INSERT INTO resource_type (workspace_id, name, schema, description, created_by, format_extension, edited_at)
+VALUES (
+    'admins',
+    'ai_review_instruction',
+    '{"type": "object", "properties": {"content": {"type": "string"}}}',
+    'Instructions for the AI review of deployment requests that apply to every changed item under the folder this resource lives in: an ai_review_instruction at f/billing/REVIEW is given to the reviewer whenever a fork''s changes touch something under f/billing/. It is read from the workspace the changes are merged into. The file body is the instructions the review follows.',
+    'system',
+    'md',
+    now()
+)
+ON CONFLICT (workspace_id, name) DO NOTHING;
@@ -50,8 +50,12 @@ Symbols, not line numbers, are cited: they drift less.
  delivered to the global chat of everyone who can read the resource, with no opt-in, whenever the
  chat touches a path under its folder (`copilot/chat/folderInstructions.ts`); the chat then acts
  with that reader's permissions, outside the folder too. `ai_skill` bodies are likewise in play
-  for every reader until turned off. The resource ACL is the only control, so granting write on a
-  folder grants that influence.
+  for every reader until turned off. An `ai_review_instruction` steers the AI review of every
+  deployment request touching its folder, for anyone who can read it in the parent workspace
+  (`deploymentRequest/aiReview.ts`); a fork's own copy is never read. The prompt tells the model to
+  keep reporting secrets and unintended deletions whatever they say, but that is a request to the
+  model, not enforcement: an instruction body can still talk it out of them. The resource ACL is the only
+  control, so granting write on a folder grants that influence.
 - **Restricted job tokens** (`job_token_scopes` on scripts and flows): a job's effective scopes
  are stored on its `job_perms` row at push and minted into its token
  (`create_token_for_owner`); an empty set is minted as `NO_API_ACCESS_SCOPE`, since
@@ -33,15 +33,18 @@ const PAGE_SIZE = 100
 /** Guard against a paging bug looping forever, not a product cap. */
 const MAX_PAGES = 20

-/** Every folder instruction the user can read in the workspace. Read access is the
- * resource's own ACL, so a folder's instructions reach exactly who can see them. */
-export async function listFolderInstructions(workspace: string): Promise<FolderInstruction[]> {
+/** Every folder instruction of `resourceType` the user can read in the workspace. Read
+ * access is the resource's own ACL, so a folder's instructions reach exactly who can see them. */
+export async function listFolderInstructions(
+	workspace: string,
+	resourceType: string = FOLDER_INSTRUCTIONS_RESOURCE_TYPE
+): Promise<FolderInstruction[]> {
 	if (!workspace) return []
 	const rows: FolderInstruction[] = []
 	for (let page = 1; page <= MAX_PAGES; page++) {
 		const resources = await ResourceService.listResource({
 			workspace,
-			resourceType: FOLDER_INSTRUCTIONS_RESOURCE_TYPE,
+			resourceType,
 			page,
 			perPage: PAGE_SIZE
 		})
@@ -195,6 +195,26 @@
 			{#if result.summary}
 				<div class="text-xs text-secondary">{result.summary}</div>
 			{/if}
+			<div class="text-2xs text-secondary">
+				{#if result.instructions.length > 0}
+					Applied {result.instructions.length} review instruction{result.instructions.length === 1
+						? ''
+						: 's'} from {parentWorkspaceId}: {result.instructions.join(', ')}.
+				{:else}
+					No review instructions from {parentWorkspaceId} applied.
+				{/if}
+				Only the ai_review_instruction resources you can read in {parentWorkspaceId} are used.
+			</div>
+			{#if result.instructionsUnavailable}
+				<Alert type="warning" title="Review instructions unavailable" size="xs">
+					Could not list the review instructions of {parentWorkspaceId}, so none were applied.
+				</Alert>
+			{/if}
+			{#if result.skippedInstructions.length > 0}
+				<Alert type="warning" title="Review instructions skipped" size="xs">
+					Could not load: {result.skippedInstructions.join(', ')}
+				</Alert>
+			{/if}
 			{#if result.unreadable.length > 0}
 				<Alert type="warning" title="Not reviewed" size="xs">
 					Could not load the contents of: {result.unreadable.join(', ')}
@@ -1,7 +1,14 @@
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 import type { WorkspaceItemDiff } from '$lib/gen'

-const h = vi.hoisted(() => ({ prompt: '', workspace: '' as string | undefined, reads: 0 }))
+const h = vi.hoisted(() => ({
+	prompt: '',
+	system: '',
+	workspace: '' as string | undefined,
+	reads: 0,
+	instructionReads: [] as string[],
+	listed: [] as string[]
+}))

 vi.mock('$lib/components/copilot/lib', () => ({
 	getNonStreamingCompletion: async (
@@ -9,6 +16,7 @@ vi.mock('$lib/components/copilot/lib', () => ({
 		_abort: AbortController,
 		options?: { workspace?: string }
 	) => {
+		h.system = messages[0].content
 		h.prompt = messages[1].content
 		h.workspace = options?.workspace
 		return '{"summary":"s","findings":[]}'
@@ -23,6 +31,24 @@ vi.mock('$lib/utils_workspace_deploy', () => ({
 	}
 }))

+vi.mock('$lib/gen', async (importOriginal) => ({
+	...(await importOriginal<typeof import('$lib/gen')>()),
+	ResourceService: {
+		listResource: async ({ workspace }: { workspace: string }) => {
+			h.listed.push(workspace)
+			if (workspace === 'offline') throw new Error('unreachable')
+			return workspace === 'parent'
+				? [{ path: 'f/a/REVIEW' }, { path: 'f/other/REVIEW' }, { path: 'f/a/gone' }]
+				: [{ path: 'f/a/FORK_REVIEW' }]
+		},
+		getResourceValue: async ({ workspace, path }: { workspace: string; path: string }) => {
+			h.instructionReads.push(`${workspace}:${path}`)
+			if (path === 'f/a/gone') throw new Error('not found')
+			return { content: `body of ${path}` }
+		}
+	}
+}))
+
 const { parseReviewAnswer, reviewDeployment } = await import('./aiReview')

 function diff(path: string): WorkspaceItemDiff {
@@ -39,6 +65,8 @@ function diff(path: string): WorkspaceItemDiff {

 beforeEach(() => {
 	h.reads = 0
+	h.instructionReads = []
+	h.listed = []
 })

 describe('parseReviewAnswer', () => {
@@ -81,4 +109,21 @@ describe('reviewDeployment', () => {
 		expect(h.reads).toBe(12)
 		expect(result.omitted).toContain('script:f/big/29')
 	})
+
+	it("follows the parent's instructions covering a changed item, and no others", async () => {
+		const result = await reviewDeployment([diff('f/a/b')], 'fork', 'parent', new AbortController())
+		expect(result.instructions).toEqual(['f/a/REVIEW'])
+		expect(result.skippedInstructions).toEqual(['f/a/gone'])
+		expect(h.system).toContain('<folder_instructions path="f/a/REVIEW" scope="f/a/">')
+		expect(h.system).toContain('body of f/a/REVIEW')
+		expect(h.system).not.toContain('f/other/REVIEW')
+		expect(h.listed).toEqual(['parent'])
+		expect(h.instructionReads.every((r) => r.startsWith('parent:'))).toBe(true)
+	})
+
+	it('reviews without instructions when the parent cannot be listed', async () => {
+		const result = await reviewDeployment([diff('f/a/b')], 'fork', 'offline', new AbortController())
+		expect(result.instructionsUnavailable).toBe(true)
+		expect(result.instructions).toEqual([])
+	})
 })
@@ -2,6 +2,19 @@ import type { AIProviderModel, WorkspaceItemDiff } from '$lib/gen'
 import { getNonStreamingCompletion } from '$lib/components/copilot/lib'
 import { getItemValue } from '$lib/utils_workspace_deploy'
 import type { Kind } from '$lib/utils_deployable'
+import {
+	formatFolderInstructions,
+	instructionsCovering,
+	listFolderInstructions,
+	readFolderInstructionBody,
+	type FolderInstruction
+} from '$lib/components/copilot/chat/folderInstructions'
+
+/**
+ * Review instructions are resources of this type in the parent workspace: a markdown file
+ * resource whose `value.content` applies to every changed item under the directory it lives in.
+ */
+export const REVIEW_INSTRUCTIONS_RESOURCE_TYPE = 'ai_review_instruction'

 export type AiReviewSeverity = 'high' | 'medium' | 'low'

@@ -20,6 +33,12 @@ export type AiReviewResult = {
 	omitted: string[]
 	/** Changed items whose contents failed to load, so the model did not review them. */
 	unreadable: string[]
+	/** Review instructions of the parent that covered a changed item and were given to the model. */
+	instructions: string[]
+	/** Review instructions listed in the parent whose body failed to load. */
+	skippedInstructions: string[]
+	/** The parent's review instructions could not be listed, so none were applied. */
+	instructionsUnavailable: boolean
 }

 // A variable's value is read decrypted and a resource's value holds credentials, so neither may
@@ -46,6 +65,54 @@ Do not comment on style, naming, formatting or lockfiles, and do not praise. Do
 Answer with JSON only, no prose around it, in this shape:
 {"summary": "<one or two sentences on what the change does and its overall risk>", "findings": [{"kind": "<item kind as given>", "path": "<item path as given>", "severity": "high" | "medium" | "low", "comment": "<the issue and what to do about it, under 80 words>"}]}`

+// Read from the parent, never the fork: the fork is the author's side, and its copy of an
+// instruction is itself one of the changes under review. A failed listing reviews without
+// instructions rather than failing the review, and is reported as `unavailable`.
+async function reviewInstructions(
+	diffs: WorkspaceItemDiff[],
+	parentWorkspaceId: string
+): Promise<{ text: string; applied: string[]; skipped: string[]; unavailable: boolean }> {
+	let listed: FolderInstruction[]
+	try {
+		listed = await listFolderInstructions(parentWorkspaceId, REVIEW_INSTRUCTIONS_RESOURCE_TYPE)
+	} catch (e) {
+		console.error('Failed to list review instructions', e)
+		return { text: '', applied: [], skipped: [], unavailable: true }
+	}
+	const covering = instructionsCovering(
+		listed,
+		diffs.map((d) => d.path)
+	)
+	const reads = await Promise.all(
+		covering.map(async (instruction) => {
+			try {
+				return {
+					instruction,
+					body: await readFolderInstructionBody(parentWorkspaceId, instruction.path)
+				}
+			} catch (e) {
+				console.error(`Failed to read review instructions ${instruction.path}`, e)
+				return { instruction, body: undefined }
+			}
+		})
+	)
+	const blocks = reads.filter(
+		(r): r is { instruction: FolderInstruction; body: string } => r.body !== undefined
+	)
+	// Hardcoded secrets and unintended deletions stay a floor: a folder's writers steer the
+	// review of their own changes, so they must not be able to switch those checks off.
+	const text =
+		blocks.length === 0
+			? ''
+			: `\n\nThe parent workspace gives the review instructions below, each for the changed items under the folder in its scope. For those items they take precedence over the instructions above, and a nested folder's over its parent folder's. They may ask you to check more, check less or weigh severity differently, except that hardcoded secrets or credentials and deletions that look unintended are always reported. The answer is still the JSON above.\n\n${formatFolderInstructions(blocks)}`
+	return {
+		text,
+		applied: blocks.map((b) => b.instruction.path),
+		skipped: reads.filter((r) => r.body === undefined).map((r) => r.instruction.path),
+		unavailable: false
+	}
+}
+
 function changeType(diff: WorkspaceItemDiff): 'added' | 'deleted' | 'modified' {
 	if (!diff.exists_in_source) return 'added'
 	if (!diff.exists_in_fork) return 'deleted'
@@ -96,7 +163,7 @@ function isEmptyRead(value: unknown): boolean {
 export function parseReviewAnswer(
 	text: string,
 	diffs: WorkspaceItemDiff[]
-): Omit<AiReviewResult, 'omitted' | 'unreadable'> {
+): Pick<AiReviewResult, 'summary' | 'findings'> {
 	const start = text.indexOf('{')
 	const end = text.lastIndexOf('}')
 	if (start === -1 || end <= start) {
@@ -137,6 +204,7 @@ export async function reviewDeployment(
 		model?: AIProviderModel
 	}
 ): Promise<AiReviewResult> {
+	const instructionsPromise = reviewInstructions(diffs, parentWorkspaceId)
 	const included: string[] = []
 	const omitted: string[] = []
 	const unreadable: string[] = []
@@ -170,9 +238,11 @@ export async function reviewDeployment(
 		user += `\n\nAlso changed, contents not shown for size:\n${omitted.map((o) => `- ${o}`).join('\n')}`
 	}

+	const instructions = await instructionsPromise
+	if (abortController.signal.aborted) throw new Error('aborted')
 	const answer = await getNonStreamingCompletion(
 		[
-			{ role: 'system', content: SYSTEM_PROMPT },
+			{ role: 'system', content: SYSTEM_PROMPT + instructions.text },
 			{ role: 'user', content: user }
 		],
 		abortController,
@@ -184,5 +254,12 @@ export async function reviewDeployment(
 			workspace: forkWorkspaceId
 		}
 	)
-	return { ...parseReviewAnswer(answer ?? '', diffs), omitted, unreadable }
+	return {
+		...parseReviewAnswer(answer ?? '', diffs),
+		omitted,
+		unreadable,
+		instructions: instructions.applied,
+		skippedInstructions: instructions.skipped,
+		instructionsUnavailable: instructions.unavailable
+	}
 }