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 + } }