diff --git a/packages/deploy/src/preflight.ts b/packages/deploy/src/preflight.ts index 6b9699f1..a1c05b80 100644 --- a/packages/deploy/src/preflight.ts +++ b/packages/deploy/src/preflight.ts @@ -2,6 +2,7 @@ import { stat } from 'node:fs/promises'; import path from 'node:path'; import { KNOWN_TRIGGER_PROVIDER_ALIASES, + lintScopes, lintTriggers, type AgentSpec } from '@agentworkforce/persona-kit'; @@ -97,9 +98,15 @@ export async function preflightPersona(personaPath: string): Promise `${issue.path}: ${issue.message}` - ); + // Scope lints ride the same warning channel as trigger lints: surfaced at + // deploy time, non-fatal. A bad scope mirrors nothing and the agent reads an + // empty tree without erroring, so a warning here is the only place it gets + // said before the author is debugging a silently inert agent. + const scopeLint = lintScopes(persona); + const warnings = [ + ...triggerLint.map((issue) => `${issue.path}: ${issue.message}`), + ...scopeLint.map((issue) => issue.message) + ]; return { persona, diff --git a/packages/persona-kit/src/index.ts b/packages/persona-kit/src/index.ts index 1c7819fd..5719af61 100644 --- a/packages/persona-kit/src/index.ts +++ b/packages/persona-kit/src/index.ts @@ -160,6 +160,12 @@ export { type TriggerLintIssue, type TriggerLintLevel } from './triggers.js'; +export { + lintScopes, + type ScopeLintCode, + type ScopeLintIssue, + type ScopeLintLevel +} from './scopes.js'; // Skill materialization export { diff --git a/packages/persona-kit/src/scopes.test.ts b/packages/persona-kit/src/scopes.test.ts new file mode 100644 index 00000000..49ef7e38 --- /dev/null +++ b/packages/persona-kit/src/scopes.test.ts @@ -0,0 +1,166 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { lintScopes } from './scopes.js'; +import type { PersonaSpec } from './types.js'; + +function persona(integrations: Record): PersonaSpec { + return { id: 'demo', integrations } as unknown as PersonaSpec; +} + +test('a concrete subpath scope is clean', () => { + const issues = lintScopes( + persona({ + linear: { scope: { projects: '/linear/projects/**', issues: '/linear/issues/**' } }, + slack: { scope: { channel: '/slack/channels/C0B9Z4CLG1J/**' } } + }) + ); + assert.deepEqual(issues, []); +}); + +test('an integration with no scope at all is clean', () => { + // Credential-only providers (an MCP server) have no Relayfile side, so + // omitting scope is correct and must not warn. + assert.deepEqual(lintScopes(persona({ 'supabase-mcp': {} })), []); +}); + +test('provider filter metadata is left alone', () => { + // `scope` is overloaded: PersonaIntegrationConfig documents it as + // provider-specific filter metadata, and these are the documented examples. + // Treating them as malformed paths would warn on every github/notion/linear + // persona in the fleet, which is exactly how a warning channel dies. + const issues = lintScopes( + persona({ + github: { scope: { repo: 'org/repo' } }, + notion: { scope: { database: 'abc123' } }, + linear: { scope: { team: 'ENG' } } + }) + ); + assert.deepEqual(issues, []); +}); + +test('an empty scope object is not flagged — the parser drops it first', () => { + // parseIntegrationConfig only assigns out.scope when the parsed map is + // non-empty, so by deploy time `scope: {}` is indistinguishable from an + // omitted scope. Warning here would advertise protection that does not exist. + assert.deepEqual(lintScopes(persona({ slack: { scope: {} } })), []); +}); + +test('an empty scope VALUE is flagged', () => { + // Unlike `scope: {}`, an empty string survives parseStringMap verbatim and + // reaches the mount, where it matches nothing under either interpretation. + const issues = lintScopes(persona({ slack: { scope: { channels: ' ' } } })); + assert.equal(issues.length, 1); + assert.equal(issues[0].code, 'scope_empty_value'); +}); + +test('a padded path is flagged, not silently accepted', () => { + // parseStringMap does not trim, so deploy forwards the padded string. Linting + // a trimmed copy would pass a value the mount then fails to match. + const issues = lintScopes(persona({ slack: { scope: { c: ' /slack/channels/C1/** ' } } })); + assert.equal(issues.length, 1); + assert.equal(issues[0].code, 'scope_untrimmed'); +}); + +test('a mid-path wildcard is flagged — the mount rejects it silently', () => { + // cloud's mount-intent allows ONLY a terminal /**; anything else mirrors + // nothing and the agent reads an empty tree without an error. + const issues = lintScopes(persona({ slack: { scope: { msgs: '/slack/*/messages' } } })); + assert.equal(issues.length, 1); + assert.equal(issues[0].code, 'scope_mid_path_wildcard'); + assert.equal(issues[0].path, 'integrations.slack.scope.msgs'); +}); + +test('a provider-root mirror is flagged as a cost, not an error', () => { + const issues = lintScopes(persona({ slack: { scope: { all: '/slack/**' } } })); + assert.equal(issues.length, 1); + assert.equal(issues[0].code, 'scope_provider_root'); + assert.equal(issues[0].level, 'warning'); + assert.match(issues[0].message, /mirrors the whole slack tree/u); +}); + +test('trailing-slash and traversal paths are flagged', () => { + const cases: [string, string][] = [ + ['/slack/channels/', 'scope_trailing_slash'], + ['/slack/../linear/issues/**', 'scope_traversal_segment'] + ]; + for (const [value, code] of cases) { + const issues = lintScopes(persona({ slack: { scope: { x: value } } })); + assert.equal(issues.length, 1, `expected one issue for ${value}`); + assert.equal(issues[0].code, code, `wrong code for ${value}`); + } +}); + +test('a path missing its leading slash is NOT flagged', () => { + // Ambiguous by construction: `slack/channels/**` may be a forgotten anchor or + // deliberate filter metadata, and nothing in the value distinguishes them. + // The lint stays silent rather than guess — see `provider filter metadata`. + assert.deepEqual(lintScopes(persona({ slack: { scope: { x: 'slack/channels/**' } } })), []); +}); + +test('never throws on malformed personas', () => { + assert.deepEqual(lintScopes({ id: 'x' } as unknown as PersonaSpec), []); + assert.deepEqual(lintScopes(persona({ slack: 'nope' as unknown as object })), []); + assert.deepEqual(lintScopes(persona({ slack: { scope: { n: 42 as unknown as string } } })), []); +}); + +test('a bounded collection wildcard is NOT flagged', () => { + // The lint does not flag terminal /** in general — whether a collection is + // affordable depends on the workspace, not the glob, and flagging every + // `/linear/issues/**` would be noise that trains authors to ignore it. + assert.deepEqual(lintScopes(persona({ linear: { scope: { i: '/linear/issues/**' } } })), []); +}); + +test('a history-sized collection wildcard IS flagged', () => { + // The exception to the rule above, and the reason this lint was written: + // `/slack/channels/**` is syntactically identical to `/linear/issues/**` but + // mirrored ~5,950 entries in a real workspace and could not converge inside + // the mount budget. Only collections that grow with history, not with + // configuration, are on the list. + const issues = lintScopes(persona({ slack: { scope: { channels: '/slack/channels/**' } } })); + assert.equal(issues.length, 1); + assert.equal(issues[0].code, 'scope_high_cardinality_root'); + assert.match(issues[0].message, /only WRITES here/u); +}); + +test('a picker-gated collection is not flagged — cloud narrows it at deploy', () => { + // The supported fix, and the one that keeps the channel choice with the + // operator: cloud rewrites this to /slack/channels//** when the + // integration is gated by an input carrying a matching picker. Warning here + // would flag the correct configuration. + const spec = { + id: 'x', + integrations: { + slack: { + optional: true, + enabledByInput: 'SLACK_CHANNEL', + scope: { channels: '/slack/channels/**' } + } + }, + inputs: { SLACK_CHANNEL: { picker: { provider: 'slack', resource: 'channels' } } } + } as unknown as PersonaSpec; + assert.deepEqual(lintScopes(spec), []); +}); + +test('a gate without a matching picker is still flagged', () => { + // The rewrite is picker-driven; `enabledByInput` alone does not narrow + // anything, so the broad mount is still paid and still worth warning about. + const spec = { + id: 'x', + integrations: { + slack: { optional: true, enabledByInput: 'SLACK_CHANNEL', scope: { channels: '/slack/channels/**' } } + }, + inputs: { SLACK_CHANNEL: { description: 'no picker' } } + } as unknown as PersonaSpec; + const issues = lintScopes(spec); + assert.equal(issues.length, 1); + assert.equal(issues[0].code, 'scope_high_cardinality_root'); +}); + +test('a single entry under a history-sized collection is clean', () => { + // The fix the warning above points at: scoping the one channel the agent + // posts to must not itself warn, or the advice is unfollowable. + assert.deepEqual( + lintScopes(persona({ slack: { scope: { channel: '/slack/channels/C0B9Z4CLG1J/**' } } })), + [] + ); +}); diff --git a/packages/persona-kit/src/scopes.ts b/packages/persona-kit/src/scopes.ts new file mode 100644 index 00000000..2722df12 --- /dev/null +++ b/packages/persona-kit/src/scopes.ts @@ -0,0 +1,288 @@ +import type { PersonaSpec } from './types.js'; + +/** + * Lint a persona's integration mount `scope` globs. + * + * ## Why this exists + * + * `scope` decides two things at once: which Relayfile paths the sandbox + * mirrors, and what the runtime token is allowed to touch. Both failure modes + * are SILENT at runtime — a scope the mount rejects produces an empty mirror, + * so reads come back empty and writebacks land on unmounted disk as no-ops. + * Nothing throws. The agent simply does nothing and reports success. + * + * Every path rule below mirrors the authoritative validation in cloud's + * `relayfile/mount-intent.ts`, so a path this lint rejects is one the mount + * would also reject. The lint runs at deploy time, where the author can still + * fix it, rather than leaving them to infer it from an agent that quietly + * read nothing. + * + * ## Why only `/`-leading values are checked + * + * `scope` is overloaded. {@link PersonaIntegrationConfig} documents it as + * "provider-specific filter metadata" — `{ repo: 'org/repo' }` for github, + * `{ database: '' }` for notion, `{ team: 'ENG' }` for linear — while the + * cloud-persona docs use it for Relayfile mount globs. Both are real and in use. + * + * A value that does not start with `/` is therefore filter metadata, not a + * malformed path, and this lint says nothing about it. There is no way to tell + * `'org/repo'` (correct) from a path someone forgot to anchor, and guessing + * wrong would fire a warning on every github and notion persona in the fleet. + * A warning channel only works while every warning is worth reading, so this + * one stays silent where it cannot be sure. + */ +export type ScopeLintLevel = 'warning'; + +/** + * Machine-readable issue category, so callers can branch on `issue.code` + * without parsing the human-readable `message`. + */ +export type ScopeLintCode = + | 'scope_empty_value' + | 'scope_untrimmed' + | 'scope_trailing_slash' + | 'scope_mid_path_wildcard' + | 'scope_traversal_segment' + | 'scope_provider_root' + | 'scope_high_cardinality_root'; + +export interface ScopeLintIssue { + level: ScopeLintLevel; + code: ScopeLintCode; + /** Provider slug the issue was raised under (`slack`, `linear`, …). */ + provider: string; + /** The scope key that was flagged (`channels`, `issues`, …). */ + resource: string; + /** Field-pointed location, e.g. `integrations.slack.scope.channels`. */ + path: string; + message: string; +} + +/** + * Collections that grow with workspace HISTORY rather than with configuration, + * so their size is unbounded from the persona author's point of view. + * + * This is a heuristic and deliberately short. It exists because the failure it + * predicts is not otherwise visible in the glob: `/slack/channels/**` is + * syntactically identical to `/linear/projects/**`, but one is a handful of + * entries and the other was ~5,950 (2,008 files / 3,940 directories) in a real + * workspace — enough that the boot mirror could not traverse it inside its + * budget, leaving the agent running degraded and later failing outright. + * + * Being on this list is not an error. A read-heavy agent may genuinely need + * the whole collection; the warning asks the author to confirm that, and points + * write-only agents at the much cheaper single-entry scope. + */ +const HIGH_CARDINALITY_ROOTS = new Set([ + 'slack/channels', + 'google-mail/messages', + 'google-mail/threads', + 'gmail/messages', + 'gmail/threads' +]); + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +/** + * Is this collection scope already narrowed for us at deploy time? + * + * Cloud rewrites a picker-gated collection scope down to the single record the + * deploy input resolves to — `/slack/channels/**` becomes + * `/slack/channels//**` for both reads and writebacks (cloud's + * `persona-deploy.ts`, `pickerTargetPath`). It fires only when the integration + * carries `enabledByInput` AND the named input carries a `picker` whose + * `provider` matches this integration and whose `resource` matches the + * collection segment being scoped. + * + * Warning on that arrangement would flag the CORRECT configuration — the one + * that preserves the operator's choice of channel instead of hard-coding an id — + * which is worse than staying quiet, so it is excluded here. + */ +function isPickerNarrowed( + persona: PersonaSpec, + provider: string, + config: Record, + collection: string +): boolean { + const gate = config.enabledByInput; + if (typeof gate !== 'string' || !gate.trim()) return false; + + const inputs = (persona as { inputs?: unknown }).inputs; + if (!isRecord(inputs)) return false; + const spec = inputs[gate.trim()]; + if (!isRecord(spec)) return false; + const picker = spec.picker; + if (!isRecord(picker)) return false; + + // Compared the same way cloud does: lowercased provider, and the resource + // stripped of surrounding slashes. + const pickerProvider = + typeof picker.provider === 'string' ? picker.provider.trim().toLowerCase() : ''; + const pickerResource = + typeof picker.resource === 'string' ? picker.resource.trim().replace(/^\/+|\/+$/gu, '') : ''; + return pickerProvider === provider.trim().toLowerCase() && pickerResource === collection; +} + +/** + * Walk a persona's `integrations[].scope` globs and flag ones the runtime + * mount would reject or silently over-mirror. Always returns; never throws. + * + * The deploy CLI surfaces these as yellow warnings before deploy and continues + * regardless — same contract as {@link lintTriggers}. None of these are fatal: + * a persona whose scope is wrong still deploys, it just will not do what its + * author expects, and the warning is the only place that gets said out loud. + */ +export function lintScopes(persona: PersonaSpec): ScopeLintIssue[] { + const issues: ScopeLintIssue[] = []; + const integrations = persona.integrations; + if (!isRecord(integrations)) return issues; + + for (const [provider, config] of Object.entries(integrations)) { + if (!isRecord(config)) continue; + + // An empty `scope: {}` is deliberately NOT flagged: `parseIntegrationConfig` + // drops it before the spec ever reaches this lint (it only assigns + // `out.scope` when the parsed map is non-empty), so by deploy time it is + // indistinguishable from an omitted scope — which is itself legitimate, as a + // credential-only provider like an MCP server has no Relayfile side at all. + // There is no signal here to warn on. + const scope = config.scope; + if (!isRecord(scope)) continue; + + for (const [resource, raw] of Object.entries(scope)) { + if (typeof raw !== 'string') continue; + const path = `integrations.${provider}.scope.${resource}`; + + // Lint the value DEPLOY sees, not a cleaned-up copy of it. `parseStringMap` + // stores scope values verbatim — no trimming — so checking `raw.trim()` + // here would clear a padded path that then fails at the mount. + const value = raw; + + if (!value.trim()) { + issues.push({ + level: 'warning', + code: 'scope_empty_value', + provider, + resource, + path, + message: + `${path} is empty, which mirrors nothing and filters nothing. Reads come ` + + `back empty and writebacks are silent no-ops. Give it the concrete path or ` + + `filter this agent needs, or drop the key.` + }); + continue; + } + + if (value !== value.trim()) { + issues.push({ + level: 'warning', + code: 'scope_untrimmed', + provider, + resource, + path, + message: + `${path}: "${value}" has leading or trailing whitespace, which is stored and ` + + `forwarded verbatim — the mount sees the padded string and will not match.` + }); + continue; + } + + // Everything below is a PATH rule. A value that does not start with "/" is + // provider filter metadata (`{ repo: 'org/repo' }`), not a malformed path, + // and is left alone — see the note on this module. + if (!value.startsWith('/')) continue; + + if (value !== '/' && value.endsWith('/')) { + issues.push({ + level: 'warning', + code: 'scope_trailing_slash', + provider, + resource, + path, + message: `${path}: "${value}" has a trailing slash, which the mount rejects.` + }); + continue; + } + + // Only a TERMINAL `/**` is allowed. A wildcard anywhere else — the + // classic `/slack/*/messages` — is rejected by the mount and the agent + // silently sees an empty tree. + const withoutGlob = value.endsWith('/**') ? value.slice(0, -3) : value; + if (/[*?{}[\]]/u.test(withoutGlob)) { + issues.push({ + level: 'warning', + code: 'scope_mid_path_wildcard', + provider, + resource, + path, + message: + `${path}: "${value}" uses a wildcard outside a terminal "/**". The mount ` + + `accepts only a terminal "/**", so this scope mirrors nothing and the ` + + `agent will read an empty tree without error.` + }); + continue; + } + + const segments = withoutGlob.split('/').slice(1); + if (segments.some((s) => s === '' || s === '.' || s === '..')) { + issues.push({ + level: 'warning', + code: 'scope_traversal_segment', + provider, + resource, + path, + message: `${path}: "${value}" contains an empty or traversal ("."/"..") segment, which the mount rejects.` + }); + continue; + } + + // Valid, but it mirrors the provider's entire tree. This is a cost + // warning rather than a correctness one: the mirror is traversed at + // sandbox boot, so a collection that grows with workspace history (Slack + // channels being the usual one) can outgrow the mount budget and leave + // the agent running degraded — or, once a deployment cancels a + // non-converging mount, failing outright. + if ( + segments.length === 2 && + value.endsWith('/**') && + HIGH_CARDINALITY_ROOTS.has(segments.join('/')) && + !isPickerNarrowed(persona, provider, config, segments[1]) + ) { + issues.push({ + level: 'warning', + code: 'scope_high_cardinality_root', + provider, + resource, + path, + message: + `${path}: "${value}" mirrors every entry under ${segments.join('/')}, which grows ` + + `with workspace history rather than with configuration. The mirror is traversed at ` + + `sandbox boot, so this can outrun the mount budget and leave the agent reading an ` + + `empty tree or failing to start. If the agent only WRITES here (posting a message, ` + + `say), scope the single entry it targets instead — the mirror is not needed for the ` + + `write, only the grant is.` + }); + continue; + } + + if (segments.length === 1 && value.endsWith('/**')) { + issues.push({ + level: 'warning', + code: 'scope_provider_root', + provider, + resource, + path, + message: + `${path}: "${value}" mirrors the whole ${provider} tree. The mirror is ` + + `traversed at sandbox boot, so this grows with workspace history and can ` + + `outrun the mount budget. Scope the concrete subpaths the handler reads ` + + `or writes back to instead.` + }); + } + } + } + + return issues; +}