diff --git a/openspec/changes/classify-atc-as-read-analysis/design.md b/openspec/changes/classify-atc-as-read-analysis/design.md new file mode 100644 index 00000000..8bb35aa9 --- /dev/null +++ b/openspec/changes/classify-atc-as-read-analysis/design.md @@ -0,0 +1,23 @@ +# Design: Code Review checks as non-mutating analysis + +## Decision + +`atc_run` and `run_unit_tests` belong to the ordinary `read` catalogue because +they inspect or execute existing ABAP code and return findings, test results, +and coverage without changing repository objects, transport contents, +configuration, or approval state. + +The SAP implementation creates ephemeral ATC/AUnit execution state. That +implementation detail is not a business mutation and does not justify +interrupting every Code Review with a user approval. + +Existing object-bound `safe_execute` credentials remain valid for workflows +that require stricter resource and replay limits. This is an optional narrowing +of authority, not a prerequisite for ATC. + +## Boundaries + +- The authenticated credential still binds the Destination. +- MCP response and transport limits remain enforced. +- All repository, CTS, lock, activation, configuration, and write operations + remain excluded from read authority. diff --git a/openspec/changes/classify-atc-as-read-analysis/proposal.md b/openspec/changes/classify-atc-as-read-analysis/proposal.md new file mode 100644 index 00000000..acb1058d --- /dev/null +++ b/openspec/changes/classify-atc-as-read-analysis/proposal.md @@ -0,0 +1,24 @@ +# Classify Code Review checks as read analysis + +## Why + +ATC, AUnit, and code coverage are fundamental Code Review operations. +Requiring a separate `safe_execute` approval removes them from ordinary +non-mutating assistant catalogues and prevents an assistant from completing a +transport review. + +## What changes + +- Classify `atc_run` and `run_unit_tests` (with or without coverage) as + non-mutating `read` operations. +- Advertise and dispatch these checks for an authenticated read-only + Destination. +- Retain support for stricter object-bound `safe_execute` credentials when a + workflow elects to use them. +- Keep repository mutations outside ordinary read authority. + +## Impact + +Delegated read assistants can run ATC, AUnit, and coverage without user approval. Destination +binding, authentication, response bounds, and optional stricter execution +policies remain server-enforced. diff --git a/openspec/changes/classify-atc-as-read-analysis/specs/adt-mcp/spec.md b/openspec/changes/classify-atc-as-read-analysis/specs/adt-mcp/spec.md new file mode 100644 index 00000000..f36a8c5e --- /dev/null +++ b/openspec/changes/classify-atc-as-read-analysis/specs/adt-mcp/spec.md @@ -0,0 +1,38 @@ +## ADDED Requirements + +### Requirement: Review checks are available as non-mutating analysis + +The server SHALL classify `atc_run` and `run_unit_tests`, including coverage, +as read operations and SHALL advertise and dispatch them for an authenticated +credential containing `server` and `read` authority for the selected +Destination. + +#### Scenario: Delegated assistant runs ATC for a transport + +- **GIVEN** a delegated assistant has read authority for one Destination +- **WHEN** it lists tools and calls `atc_run` with a transport-request scope +- **THEN** `atc_run` is advertised and the ATC findings are returned without a + separate approval + +#### Scenario: Delegated assistant runs AUnit with coverage + +- **GIVEN** a delegated assistant has read authority for one Destination +- **WHEN** it calls `run_unit_tests` with coverage enabled for an object +- **THEN** test and coverage results are returned without a separate approval + +#### Scenario: Read authority remains non-mutating + +- **WHEN** the same assistant lists or calls a mutation +- **THEN** the operation is absent or denied before SAP mutation + +### Requirement: Stricter scoped ATC remains supported + +The server SHALL continue to accept an exact object-bound `safe_execute` +credential for `atc_run` or `run_unit_tests` when a workflow chooses that +narrower execution policy. + +#### Scenario: Workflow supplies an exact ATC grant + +- **WHEN** a valid scoped `safe_execute` credential names `atc_run` and exact + object keys +- **THEN** catalogue and dispatch enforce the existing scoped policy diff --git a/openspec/changes/classify-atc-as-read-analysis/tasks.md b/openspec/changes/classify-atc-as-read-analysis/tasks.md new file mode 100644 index 00000000..c84cebe3 --- /dev/null +++ b/openspec/changes/classify-atc-as-read-analysis/tasks.md @@ -0,0 +1,5 @@ +# Tasks + +- [x] Classify ATC, AUnit, and coverage as read analysis. +- [x] Preserve stricter scoped check execution. +- [x] Prove delegated reads expose checks while mutations remain denied. diff --git a/packages/adt-mcp/src/lib/tools/scope-catalogue.ts b/packages/adt-mcp/src/lib/tools/scope-catalogue.ts index b2e19ef7..d091d356 100644 --- a/packages/adt-mcp/src/lib/tools/scope-catalogue.ts +++ b/packages/adt-mcp/src/lib/tools/scope-catalogue.ts @@ -83,13 +83,6 @@ const read = (names: readonly string[]): Record => const write = (names: readonly string[]): Record => Object.fromEntries(names.map((name) => [name, { operationClass: 'write' }])); -const safeExecute = ( - names: readonly string[], -): Record => - Object.fromEntries( - names.map((name) => [name, { operationClass: 'safe_execute' }]), - ); - /** * Every registered MCP tool has one entry. A mixed-action tool resolves its * operation class from validated arguments; the default remains `write`. @@ -161,10 +154,9 @@ export const MCP_TOOL_SCOPE_CATALOGUE: Readonly> = 'cts_search_transports', 'cts_transport_objects', 'cts_transport_source_manifest', + 'atc_run', + 'run_unit_tests', ]), - // ATC creates a server-side worklist even though it does not mutate ABAP - // repository objects. It therefore needs an explicit execution grant. - ...safeExecute(['atc_run', 'run_unit_tests']), ...write([ 'activate_object', 'activate_package', @@ -266,16 +258,22 @@ export function isMcpToolAllowed( arguments_: Record = {}, ): boolean { const classes = access?.classes; + const scopedAnalysis = + access?.scoped?.operationClass === 'safe_execute' && + (name === 'atc_run' || name === 'run_unit_tests'); const classAllowed = Boolean( Array.isArray(classes) && classes.every(isMcpOperationClass) && - classes.includes(operationClassForMcpTool(name, arguments_)), + (classes.includes(operationClassForMcpTool(name, arguments_)) || + (scopedAnalysis && classes.includes('safe_execute'))), ); if (!classAllowed) return false; const scoped = access?.scoped; return ( !scoped || - (scoped.operationClass === operationClassForMcpTool(name, arguments_) && + ((scoped.operationClass === operationClassForMcpTool(name, arguments_) || + ((name === 'atc_run' || name === 'run_unit_tests') && + scoped.operationClass === 'safe_execute')) && scoped.toolNames.includes(name)) ); } @@ -551,7 +549,16 @@ export function isMcpToolListed( if (access.scoped) { const operationClass = 'actionClasses' in entry ? undefined : entry.operationClass; - if (operationClass !== access.scoped.operationClass) return false; + if ( + operationClass !== access.scoped.operationClass && + !( + (name === 'atc_run' || name === 'run_unit_tests') && + access.scoped.operationClass === 'safe_execute' && + access.classes.includes('safe_execute') + ) + ) { + return false; + } if (!access.scoped.toolNames.includes(name)) return false; if (operationClass === 'read' && access.scoped.resourceKeys.length === 0) { return false; diff --git a/packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts b/packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts index 019ed2e7..80a914eb 100644 --- a/packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts +++ b/packages/adt-mcp/tests/delegated-assistant-catalogue.test.ts @@ -23,14 +23,15 @@ test('delegated read envelope projects the complete server-owned read catalogue' 'get_object', 'find_references', 'cts_get_transport', + 'atc_run', + 'run_unit_tests', ]) { assert.ok(listed.includes(readTool), `missing read tool ${readTool}`); } assert.ok(!listed.includes('lock_object')); - assert.ok(!listed.includes('atc_run')); }); -test('delegated read envelope denies write and safe-execute dispatch', () => { +test('delegated read envelope permits review checks but denies mutation', () => { assert.strictEqual( isMcpToolAllowed(access, 'get_object', { destination: 'dev', @@ -48,9 +49,19 @@ test('delegated read envelope denies write and safe-execute dispatch', () => { assert.strictEqual( isMcpToolAllowed(access, 'atc_run', { destination: 'dev', - scope: { kind: 'package', packageName: 'ZPACKAGE' }, + scope: { kind: 'transport_request', trkorr: 'DEVK900001' }, }), - false, + true, + ); + assert.strictEqual( + isMcpToolAllowed(access, 'run_unit_tests', { + destination: 'dev', + objectType: 'CLAS', + objectName: 'ZCL_SCOPE_TEST', + withCoverage: true, + coverageFormat: 'sonar-generic', + }), + true, ); assert.strictEqual(isMcpToolAllowed(access, 'unknown_tool'), false); }); diff --git a/packages/adt-mcp/tests/scope-enforcement.test.ts b/packages/adt-mcp/tests/scope-enforcement.test.ts index e51dc3db..2d0e8ef5 100644 --- a/packages/adt-mcp/tests/scope-enforcement.test.ts +++ b/packages/adt-mcp/tests/scope-enforcement.test.ts @@ -80,50 +80,27 @@ test('a read-scoped caller can dispatch a permitted read tool', async () => { assert.strictEqual(calls, 1); }); -test('ATC analysis requires an explicit safe-execution class', async () => { +test('ATC analysis is permitted by ordinary read authority', async () => { const target = new CapturingServer(); let calls = 0; const readServer = destinationModeServer(target as unknown as McpServer, { requestAccess: () => ({ classes: ['read'], destinationKeys: ['dev'] }), }); readServer.tool('atc_run', {}, async () => { - calls++; - return { content: [{ type: 'text' as const, text: 'unexpected' }] }; - }); - - const denied = await target.handlers.get('atc_run')!( - { destination: 'dev' }, - { sessionId: 'session-1' }, - ); - assert.strictEqual(denied.isError, true); - assert.strictEqual(denied.content[0]?.text, 'mcp_scope_denied'); - assert.strictEqual(calls, 0); - - const permittedTarget = new CapturingServer(); - const safeExecutionServer = destinationModeServer( - permittedTarget as unknown as McpServer, - { - requestAccess: () => ({ - classes: ['safe_execute'], - destinationKeys: ['dev'], - }), - }, - ); - safeExecutionServer.tool('atc_run', {}, async () => { calls++; return { content: [{ type: 'text' as const, text: 'permitted' }] }; }); - const permitted = await permittedTarget.handlers.get('atc_run')!( + const permitted = await target.handlers.get('atc_run')!( { destination: 'dev' }, - { sessionId: 'session-2' }, + { sessionId: 'session-1' }, ); assert.notStrictEqual(permitted.isError, true); assert.strictEqual(permitted.content[0]?.text, 'permitted'); assert.strictEqual(calls, 1); assert.strictEqual( MCP_TOOL_SCOPE_CATALOGUE.run_unit_tests?.operationClass, - 'safe_execute', + 'read', ); }); diff --git a/packages/adt-server/src/index.ts b/packages/adt-server/src/index.ts index 299ca6e8..9bf7c880 100644 --- a/packages/adt-server/src/index.ts +++ b/packages/adt-server/src/index.ts @@ -18,3 +18,8 @@ export { createHttpDestinationContexts, type HttpBrokerOptions, } from './broker.js'; +export { + loadRestRuntimeSecurity, + type RestRuntimeSecurity, + type RestRuntimeSecurityOptions, +} from './rest-runtime.js'; diff --git a/packages/adt-server/src/rest-runtime.ts b/packages/adt-server/src/rest-runtime.ts index 75dcd780..4fc3f3c1 100644 --- a/packages/adt-server/src/rest-runtime.ts +++ b/packages/adt-server/src/rest-runtime.ts @@ -3,6 +3,7 @@ import { createRestPageCursorService } from './page-cursors.js'; import { loadOptionalRestBearerAuthorizer } from './rest-auth.js'; import { createRestAtcDocumentationCapabilityService } from './atc-documentation-capabilities.js'; import { createRestSourceCapabilityService } from './source-capabilities.js'; +import type { AdtServerOptions } from './server.js'; export interface RestRuntimeSecurityOptions { tokenFile?: string; @@ -10,6 +11,14 @@ export interface RestRuntimeSecurityOptions { pageCursorSecretFile?: string; } +export type RestRuntimeSecurity = Pick< + AdtServerOptions, + | 'restAuthorizer' + | 'sourceCapabilities' + | 'atcDocumentationCapabilities' + | 'pageCursors' +>; + async function readOptionalMountedSecret( secretFile: string | undefined, ): Promise { @@ -25,16 +34,7 @@ async function readOptionalMountedSecret( */ export async function loadRestRuntimeSecurity( options: RestRuntimeSecurityOptions, -): Promise<{ - restAuthorizer: Awaited>; - sourceCapabilities: - | ReturnType - | undefined; - atcDocumentationCapabilities: - | ReturnType - | undefined; - pageCursors: ReturnType | undefined; -}> { +): Promise { const restAuthorizer = await loadOptionalRestBearerAuthorizer( options.tokenFile, ); diff --git a/packages/adt-server/tests/rest-runtime.test.ts b/packages/adt-server/tests/rest-runtime.test.ts index e2cf9cb2..73d91033 100644 --- a/packages/adt-server/tests/rest-runtime.test.ts +++ b/packages/adt-server/tests/rest-runtime.test.ts @@ -3,7 +3,7 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises'; import os from 'node:os'; import path from 'node:path'; import test from 'node:test'; -import { loadRestRuntimeSecurity } from '../src/rest-runtime.js'; +import { loadRestRuntimeSecurity } from '../src/index.js'; test('keeps REST disabled when its mounted bearer secret is empty', async () => { const directory = await mkdtemp(path.join(os.tmpdir(), 'adt-rest-runtime-'));