diff --git a/server/lib/providerVendors.js b/server/lib/providerVendors.js index a854f95ba1..708d81edc0 100644 --- a/server/lib/providerVendors.js +++ b/server/lib/providerVendors.js @@ -629,6 +629,11 @@ export function buildVendorCliArgs(provider, baseArgs, { model, effort }) { return vendor.cliArgs(baseArgs, { model, effort, provider }); } +/** How a provider names itself in an error a user has to act on. */ +function providerLabel(provider) { + return provider?.id || provider?.command || 'unknown'; +} + /** * `buildCliSpawnConfig` (agentCliSpawning.js): full `{ command, args, * stdinMode, streamFormat? }` shape per vendor. Requires `spawnArgs` to be @@ -641,7 +646,7 @@ export function buildVendorSpawnConfig(provider, ctx) { if (posture) { const recipe = publicReviewRecipe(provider, posture); if (!recipe) { - throw new Error(`Provider '${provider?.id || provider?.command || 'unknown'}' has no enforced ${posture} public-review posture`); + throw new Error(`Provider '${providerLabel(provider)}' has no enforced ${posture} public-review posture`); } return recipe.spawnArgs(provider, ctx); } @@ -678,6 +683,30 @@ export function supportsPublicReviewPosture(provider, posture, { tui = false } = return Boolean(publicReviewRecipe(provider, posture)); } +/** + * The spawn-time gate for a public-content stage, as a block or `null`. + * + * Takes the POSTURE (what the stage requires), not a boolean, because the + * caller's posture is `null` for every ordinary task — and `null` has no + * recipe, so asking `supportsPublicReviewPosture` about it answers "false" + * for work that was never public-content at all. Deciding here keeps the + * "no posture requested" case explicit instead of a caller-side `&&` that a + * refactor can drop (it was, in #5830: every ordinary agent task blocked with + * "has no enforced null public-content review mode"). + * + * @returns {{ reason: string, category: string }|null} + */ +export function publicReviewProviderBlock(provider, posture, { tui = false } = {}) { + if (!posture) return null; + if (supportsPublicReviewPosture(provider, posture, { tui })) return null; + return { + reason: `Provider '${providerLabel(provider)}' has no enforced ${posture} public-content review mode`, + category: posture === PUBLIC_REVIEW_ACTIONS_POSTURE + ? 'public-review-actions-provider-unsupported' + : 'public-review-provider-unsupported', + }; +} + /** Whether a provider can run a tool-free public-content stage. */ export function supportsPublicReviewProvider(provider, options) { return supportsPublicReviewPosture(provider, PUBLIC_REVIEW_NO_TOOL_POSTURE, options); diff --git a/server/lib/providerVendors.publicReview.test.js b/server/lib/providerVendors.publicReview.test.js index 1fabe5b8e6..3375c2d18e 100644 --- a/server/lib/providerVendors.publicReview.test.js +++ b/server/lib/providerVendors.publicReview.test.js @@ -9,6 +9,7 @@ import { import { buildVendorSpawnConfig, publicReviewPosturesForProvider, + publicReviewProviderBlock, supportsPublicReviewProvider, supportsPublicReviewActionsProvider, } from './providerVendors.js'; @@ -40,6 +41,37 @@ describe('public-review provider postures', () => { expect(publicReviewPosturesForProvider(localClaude)).toEqual([PUBLIC_REVIEW_NO_TOOL_POSTURE]); }); + // Regression (#5830): the spawn gate passes the posture a STAGE requires, + // which is `null` for every ordinary agent task. Answering "unsupported" for + // a task that requested no posture blocked all normal work — scheduled tasks + // included — with "has no enforced null public-content review mode". + it('does not block a task that requested no posture', () => { + expect(publicReviewProviderBlock(codex, null)).toBeNull(); + expect(publicReviewProviderBlock(codex, undefined)).toBeNull(); + // The transport is irrelevant when nothing is being enforced: a TUI session + // and an api provider run ordinary tasks all day. + expect(publicReviewProviderBlock({ ...codex, type: 'tui' }, null, { tui: true })).toBeNull(); + expect(publicReviewProviderBlock({ ...codex, type: 'api' }, null)).toBeNull(); + }); + + it('blocks a requested posture the provider has no recipe for, naming that posture', () => { + expect(publicReviewProviderBlock(codex, PUBLIC_REVIEW_NO_TOOL_POSTURE)).toBeNull(); + expect(publicReviewProviderBlock(codex, PUBLIC_REVIEW_ACTIONS_POSTURE)).toBeNull(); + + // claude has permission modes but no sandbox flag — tool-free only. + expect(publicReviewProviderBlock(localClaude, PUBLIC_REVIEW_NO_TOOL_POSTURE)).toBeNull(); + expect(publicReviewProviderBlock(localClaude, PUBLIC_REVIEW_ACTIONS_POSTURE)).toEqual({ + reason: "Provider 'claude-ollama' has no enforced sandboxed-actions public-content review mode", + category: 'public-review-actions-provider-unsupported', + }); + + // A TUI session has no enforced argv, whatever its vendor row declares. + expect(publicReviewProviderBlock({ ...codex, type: 'tui' }, PUBLIC_REVIEW_NO_TOOL_POSTURE, { tui: true })).toEqual({ + reason: "Provider 'codex-cli' has no enforced no-tool public-content review mode", + category: 'public-review-provider-unsupported', + }); + }); + it('fails closed for transports and vendors with no maintained recipe', () => { // A TUI session and an HTTP api provider have no enforced argv at all. expect(publicReviewPosturesForProvider({ ...codex, type: 'tui' }, { tui: true })).toEqual([]); diff --git a/server/services/agentLifecycle.js b/server/services/agentLifecycle.js index 8c1feb14c6..c0a33fd070 100644 --- a/server/services/agentLifecycle.js +++ b/server/services/agentLifecycle.js @@ -62,7 +62,7 @@ import { cliProviderAuthDescriptor } from '../lib/processEnv.js'; import { PROVIDER_TYPES } from '../lib/aiToolkit/constants.js'; import { buildCliSpawnConfig, isClaudeCliProvider, isTuiProvider, getClaudeSettingsEnv, spawnDirectly } from './agentCliSpawning.js'; import { buildTuiSpawnConfig, spawnTuiAgent } from './agentTuiSpawning.js'; -import { supportsPublicReviewPosture, publicReviewPostureForProfile, PUBLIC_REVIEW_NO_TOOL_POSTURE } from '../lib/providerVendors.js'; +import { publicReviewProviderBlock, publicReviewPostureForProfile, PUBLIC_REVIEW_NO_TOOL_POSTURE } from '../lib/providerVendors.js'; import { PUBLIC_REVIEW_ACTIONS_EXECUTION_PROFILE } from '../lib/agentExecutionProfiles.js'; import { formatPublicReviewInputPrompt } from '../lib/modelAbuseGuard.js'; import { materializePublicReviewInput, materializePublicReviewPatches, readPublicReviewInputSnapshot, validatePublicReviewModel } from './modelAbuseGuard.js'; @@ -463,15 +463,17 @@ async function runAgentSpawn(task) { // One posture check for both stages. Eligibility is declared by the vendor // row and re-asserted HERE, at spawn time, because a schedule or API // payload can be edited without the browser: the picker is a convenience, - // never the enforcement. - if (!supportsPublicReviewPosture(provider, publicReviewPosture, { tui: isTui })) { - const reason = `Provider '${provider?.id || provider?.command || 'unknown'}' has no enforced ${publicReviewPosture} public-content review mode`; + // never the enforcement. The helper owns the "no posture requested" case, + // so an ordinary task (posture `null`) passes straight through (#5830). + const postureBlock = publicReviewProviderBlock(provider, publicReviewPosture, { tui: isTui }); + if (postureBlock) { + const { reason, category } = postureBlock; await updateTask(task.id, { status: 'blocked', metadata: { ...task.metadata, blockedReason: reason, - blockedCategory: publicReviewActions ? 'public-review-actions-provider-unsupported' : 'public-review-provider-unsupported', + blockedCategory: category, blockedAt: new Date().toISOString(), }, }, task.taskType || 'user').catch(() => {}); diff --git a/server/services/agentLifecycle.test.js b/server/services/agentLifecycle.test.js index 33c323c43e..3cfd802331 100644 --- a/server/services/agentLifecycle.test.js +++ b/server/services/agentLifecycle.test.js @@ -308,14 +308,35 @@ describe('agentLifecycle — guard wiring', () => { expect(AGENT_LIFECYCLE_SRC).toContain('public-review-security-scan-incomplete'); expect(AGENT_LIFECYCLE_SRC).toContain('public-review-no-cleared-prs'); expect(AGENT_LIFECYCLE_SRC).toContain('public-review-eligibility-incomplete'); - expect(AGENT_LIFECYCLE_SRC).toContain('public-review-actions-provider-unsupported'); + // The provider-unsupported categories moved to `publicReviewProviderBlock` + // with the gate decision itself; their exact values are asserted there + // (providerVendors.publicReview.test.js). expect(AGENT_LIFECYCLE_SRC).toMatch(/if \(scanBlock\) \{[\s\S]*?status: 'blocked'/); expect(AGENT_LIFECYCLE_SRC).toMatch(/expected fail-closed safety outcome/); const gateStart = AGENT_LIFECYCLE_SRC.indexOf('const scanBlock = publicReviewScanBlock(task)'); - const gateEnd = AGENT_LIFECYCLE_SRC.indexOf('if (!supportsPublicReviewPosture(provider, publicReviewPosture', gateStart); + const gateEnd = AGENT_LIFECYCLE_SRC.indexOf('const postureBlock = publicReviewProviderBlock(provider, publicReviewPosture', gateStart); expect(gateEnd).toBeGreaterThan(gateStart); expect(AGENT_LIFECYCLE_SRC.slice(gateStart, gateEnd)).not.toContain("cosEvents.emit('agent:error'"); }); + + // #5830 collapsed the two per-stage provider gates into one and dropped the + // `publicReview &&` guard with them, so an ORDINARY task — posture `null`, + // which no vendor declares a recipe for — was blocked at spawn with + // "has no enforced null public-content review mode". The decision now lives + // in `publicReviewProviderBlock` (unit-tested in + // providerVendors.publicReview.test.js), which returns null for a task that + // requested no posture. Pin the call so the caller cannot re-derive it from a + // boolean support check and reintroduce the same collapse. + it('asks the posture helper for the provider gate rather than re-deriving it', () => { + expect(AGENT_LIFECYCLE_SRC).toContain('const postureBlock = publicReviewProviderBlock(provider, publicReviewPosture, { tui: isTui })'); + expect(AGENT_LIFECYCLE_SRC).toMatch(/if \(postureBlock\) \{[\s\S]*?status: 'blocked'/); + // The helper owns the blocked category too, so the gate cannot pick its own. + expect(AGENT_LIFECYCLE_SRC).toContain('const { reason, category } = postureBlock;'); + // The reason text belongs to the helper — building it here means the gate + // decided for itself whether the posture was supported. + expect(AGENT_LIFECYCLE_SRC).not.toContain('public-content review mode'); + expect(AGENT_LIFECYCLE_SRC).not.toMatch(/supportsPublicReviewPosture\(/); + }); }); // ─── Coverage guard for the self-update spawn gate (issue #4124) ────────────