From 62fbd508c8ceb5336633fffd0fb9afbad6cfdee5 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 28 Oct 2026 01:25:07 +0100 Subject: [PATCH] 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 * fix: bound ai review fetches and surface unreadable items Co-Authored-By: Claude Opus 5.5 * feat: add fix in ai session and model picker to deployment ai review Co-Authored-By: Claude Opus 5.5 * feat: add workspace instructions for deployment ai review Co-Authored-By: Claude Opus 5.5 * fix: read review instructions from the parent and bind ai review to the fork Co-Authored-By: Claude Opus 5.5 * fix: pin the fix-all session preview to the fork's compare page Co-Authored-By: Claude Opus 5.5 * fix: show the protection-rule bypass toggle inline Co-Authored-By: Claude Opus 5.5 * fix: make the bypass toggle small and red Co-Authored-By: Claude Opus 5.5 * refactor: drop workspace-settings instructions from deployment ai review Co-Authored-By: Claude Opus 5.5 * feat: folder-scoped review instructions for the deployment ai review Co-Authored-By: Claude Opus 5.5 * feat: review the selected items with ai on compare and deploy Co-Authored-By: Claude Opus 5.5 * fix: keep reviewing when review instructions cannot be listed Co-Authored-By: Claude Opus 5.5 * docs: say the review instruction floor is a prompt request, not enforcement Co-Authored-By: Claude Opus 5.5 * fix: mark an ai review out of date when its items change Co-Authored-By: Claude Opus 5.5 * 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 Co-authored-by: windmill-internal-app[bot] --- ..._review_instruction_resource_type.down.sql | 6 ++ ...ai_review_instruction_resource_type.up.sql | 15 ++++ docs/auth-surface.md | 8 +- .../copilot/chat/folderInstructions.ts | 11 ++- .../deploymentRequest/AiReview.svelte | 20 +++++ .../deploymentRequest/aiReview.test.ts | 47 ++++++++++- .../components/deploymentRequest/aiReview.ts | 83 ++++++++++++++++++- 7 files changed, 180 insertions(+), 10 deletions(-) create mode 100644 backend/migrations/20261028002118_ai_review_instruction_resource_type.down.sql create mode 100644 backend/migrations/20261028002118_ai_review_instruction_resource_type.up.sql diff --git a/backend/migrations/20261028002118_ai_review_instruction_resource_type.down.sql b/backend/migrations/20261028002118_ai_review_instruction_resource_type.down.sql new file mode 100644 index 0000000000..80c4b7c48f --- /dev/null +++ b/backend/migrations/20261028002118_ai_review_instruction_resource_type.down.sql @@ -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'; diff --git a/backend/migrations/20261028002118_ai_review_instruction_resource_type.up.sql b/backend/migrations/20261028002118_ai_review_instruction_resource_type.up.sql new file mode 100644 index 0000000000..1fc67f2c5f --- /dev/null +++ b/backend/migrations/20261028002118_ai_review_instruction_resource_type.up.sql @@ -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; diff --git a/docs/auth-surface.md b/docs/auth-surface.md index 3b8cfbcfbf..3bcd90b571 100644 --- a/docs/auth-surface.md +++ b/docs/auth-surface.md @@ -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 diff --git a/frontend/src/lib/components/copilot/chat/folderInstructions.ts b/frontend/src/lib/components/copilot/chat/folderInstructions.ts index c07c85cbbd..f73706744a 100644 --- a/frontend/src/lib/components/copilot/chat/folderInstructions.ts +++ b/frontend/src/lib/components/copilot/chat/folderInstructions.ts @@ -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 { +/** 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 { 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 }) diff --git a/frontend/src/lib/components/deploymentRequest/AiReview.svelte b/frontend/src/lib/components/deploymentRequest/AiReview.svelte index 8e7cfbbdd0..e36a269c6d 100644 --- a/frontend/src/lib/components/deploymentRequest/AiReview.svelte +++ b/frontend/src/lib/components/deploymentRequest/AiReview.svelte @@ -195,6 +195,26 @@ {#if result.summary}
{result.summary}
{/if} +
+ {#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. +
+ {#if result.instructionsUnavailable} + + Could not list the review instructions of {parentWorkspaceId}, so none were applied. + + {/if} + {#if result.skippedInstructions.length > 0} + + Could not load: {result.skippedInstructions.join(', ')} + + {/if} {#if result.unreadable.length > 0} Could not load the contents of: {result.unreadable.join(', ')} diff --git a/frontend/src/lib/components/deploymentRequest/aiReview.test.ts b/frontend/src/lib/components/deploymentRequest/aiReview.test.ts index 1759919af8..d9c0973d96 100644 --- a/frontend/src/lib/components/deploymentRequest/aiReview.test.ts +++ b/frontend/src/lib/components/deploymentRequest/aiReview.test.ts @@ -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()), + 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('') + 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([]) + }) }) diff --git a/frontend/src/lib/components/deploymentRequest/aiReview.ts b/frontend/src/lib/components/deploymentRequest/aiReview.ts index 286c04a65a..1d2e2b4647 100644 --- a/frontend/src/lib/components/deploymentRequest/aiReview.ts +++ b/frontend/src/lib/components/deploymentRequest/aiReview.ts @@ -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": "", "findings": [{"kind": "", "path": "", "severity": "high" | "medium" | "low", "comment": ""}]}` +// 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 { +): Pick { const start = text.indexOf('{') const end = text.lastIndexOf('}') if (start === -1 || end <= start) { @@ -137,6 +204,7 @@ export async function reviewDeployment( model?: AIProviderModel } ): Promise { + 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 + } }