From 300ab71e8aadc88936508d9cb939c4044ba663b6 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Thu, 3 Sep 2026 22:49:42 +0000 Subject: [PATCH] test: pin that a public-review stage never dispatches through the CoS runner (#6105) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A public-review stage is direct-only, and nothing pinned that. The two halves of a vendor's `no-tool` posture are enforced in different places: the argv is declared on the vendor row and survives any dispatch path, but OpenCode's posture lives entirely in the config content `buildCliChildEnv` writes from the `safetyProfile` it is handed — and the CoS runner payload carries no `safetyProfile`. A stage that reached the runner would still look enforced (right flag, right provider, gate passed) while running tool-enabled against contributor-authored PR text. Silent, and green in CI. Extends the posture-gate suite's mock set so a public-review spawn reaches the dispatch (temp `PATHS.cosAgents`, resolved snapshot/prompt/run leaves) and adds the case: with runner mode on, the stage still goes to `spawnDirectly` carrying its `safetyProfile`, and `spawnAgentViaRunner` is never called. A control case proves runner mode genuinely routes an ordinary task to the runner in the same mock set, so the negative is not vacuous. The sibling "headless, never as a PTY session" case had the same vacuity — its negatives passed because the flow never reached a spawner at all. It now runs to the dispatch too and asserts `spawnDirectly` was called. No production change. Claude-Session: https://claude.ai/code/session_01RA3pD5YM2dukQwbZ3pC6WA --- .../agentLifecycle.postureGate.test.js | 145 +++++++++++++++++- 1 file changed, 139 insertions(+), 6 deletions(-) diff --git a/server/services/agentLifecycle.postureGate.test.js b/server/services/agentLifecycle.postureGate.test.js index 75ea24eb13..46b3574470 100644 --- a/server/services/agentLifecycle.postureGate.test.js +++ b/server/services/agentLifecycle.postureGate.test.js @@ -22,9 +22,35 @@ * * Mirrors the mock set in agentLifecycle.spawnViaRunner.test.js — the leaves are * stubbed so the real orchestrator runs. + * + * The last describe (#6105) observes the same spawn one step further in, at the + * DISPATCH: a public-review stage is direct-only, and the gate passing is not + * enough on its own. `reachDispatch()` below carries a case past the workspace + * prep the gate cases deliberately stop at. */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { afterAll, describe, it, expect, vi, beforeEach } from 'vitest'; +import { rmSync } from 'fs'; +import { join } from 'path'; +import { makePathsProxy } from '../lib/mockPathsDataRoot.js'; + +// A spawn that reaches the DISPATCH writes `prompt.txt` into `PATHS.cosAgents` +// and creates the agent directory. Re-rooted at a temp dir so this suite never +// writes into the developing install's agent archive. Allocated inside +// `vi.hoisted` because `agentLifecycle.js` reads `PATHS.cosAgents` at import +// time — a plain module-level const would still be in its temporal dead zone +// when the factory below runs. +const { TEMP_ROOT } = await vi.hoisted(async () => { + const { mkdtempSync } = await import('fs'); + const { tmpdir } = await import('os'); + const { join: joinPath } = await import('path'); + return { TEMP_ROOT: mkdtempSync(joinPath(tmpdir(), 'portos-posture-gate-')) }; +}); + +vi.mock('../lib/fileUtils.js', async () => { + const actual = await vi.importActual('../lib/fileUtils.js'); + return makePathsProxy(actual, { dataRoot: TEMP_ROOT }); +}); vi.mock('./cosRunnerClient.js', async (importOriginal) => ({ ...(await importOriginal()), @@ -69,8 +95,9 @@ vi.mock('./agentTuiSpawning.js', () => ({ spawnTuiAgent: vi.fn(), })); vi.mock('./agentProviderResolution.js', () => ({ resolveAgentProviderAndModel: vi.fn() })); -// Stops the spawn immediately AFTER the posture gate — the gate is what this -// file observes, and letting the real workspace prep run would touch git. +// By default, stops the spawn immediately AFTER the posture gate — the gate is +// what most of this file observes, and letting the real workspace prep run +// would touch git. The dispatch cases override it per test via `reachDispatch`. vi.mock('./agentWorkspacePrep.js', () => ({ prepareAgentWorkspace: vi.fn().mockResolvedValue({ outcome: 'blocked', reason: 'stop here' }), })); @@ -83,7 +110,17 @@ vi.mock('./agentCompletionCleanup.js', () => ({ runAgentCompletionCleanup: vi.fn vi.mock('./agentSummaryExtraction.js', () => ({ extractFinalSummary: vi.fn() })); vi.mock('./agentManagement.js', () => ({ handleOrphanedTask: vi.fn() })); vi.mock('./agentRunEventLog.js', () => ({ appendRunEvent: vi.fn(async () => ({ appended: true })) })); -vi.mock('./agentPromptBuilder.js', () => ({ buildAgentPrompt: vi.fn(), getAppWorkspace: vi.fn() })); +vi.mock('./agentPromptBuilder.js', () => ({ + buildAgentPrompt: vi.fn(), + getAppWorkspace: vi.fn(), + // Read by the `registerAgent` projection on the way to the dispatch; the real + // predicates are pinned in agentPromptBuilder's own suite. + inlinePrLifecycleSection: vi.fn(() => null), + isClaimFlowTask: vi.fn(() => false), +})); +// Dynamically imported mid-spawn purely to snapshot workspace context. Stubbed +// so a spawn that reaches the dispatch cannot touch the install's context store. +vi.mock('./workspaceContext.js', () => ({ snapshotOnRepoSwitch: vi.fn().mockResolvedValue(null) })); vi.mock('./agentErrorAnalysis.js', () => ({ analyzeAgentFailure: vi.fn().mockReturnValue({ category: 'startup-failure', actionable: false }), })); @@ -110,11 +147,15 @@ vi.mock('./modelAbuseGuard.js', () => ({ import { spawnAgentForTask } from './agentLifecycle.js'; import { prepareAgentWorkspace } from './agentWorkspacePrep.js'; -import { materializePublicReviewInput } from './modelAbuseGuard.js'; +import { materializePublicReviewInput, readPublicReviewInputSnapshot } from './modelAbuseGuard.js'; import { removeWorktree } from './worktreeManager.js'; import { resolveAgentProviderAndModel } from './agentProviderResolution.js'; +import { buildAgentPrompt } from './agentPromptBuilder.js'; +import { createAgentRun } from './agentRunTracking.js'; +import { buildCliSpawnConfig, isTuiProvider, spawnDirectly } from './agentCliSpawning.js'; +import { spawnAgentViaRunner } from './cosRunnerClient.js'; import { updateTask } from './cos.js'; -import { spawningTasks, runnerAgents } from './agentState.js'; +import { spawningTasks, runnerAgents, setUseRunner } from './agentState.js'; // The provider every install actually runs its CoS agents on, and the one named // in the #5866 outage: the broken gate rejected it for ordinary work. @@ -128,15 +169,50 @@ const postureBlockWrites = () => vi.mocked(updateTask).mock.calls.filter( && update.metadata.blockedReason.includes('public-content review mode'), ); +/** + * Carry a spawn PAST `prepareAgentWorkspace` — which the mock set above stops + * every other case at — so the dispatch at the end of `runAgentSpawn` is + * actually reached. Only the leaves between prep and dispatch are filled in; + * the orchestrator itself stays the production one, which is the whole point: + * the invariant under test is a single expression inside it. + * + * No `worktreeInfo`, so the run never reads a real checkout for its + * branch-jack baseline. + */ +function reachDispatch() { + vi.mocked(prepareAgentWorkspace).mockResolvedValueOnce({ + outcome: 'ready', + workspacePath: join(TEMP_ROOT, 'workspace'), + resolvedAppName: 'example-app', + worktreeInfo: null, + }); + vi.mocked(materializePublicReviewInput).mockResolvedValue(true); + vi.mocked(readPublicReviewInputSnapshot).mockResolvedValue({ pullRequests: [] }); + vi.mocked(buildAgentPrompt).mockResolvedValue('review the cleared input'); + vi.mocked(createAgentRun).mockResolvedValue({ runId: 'run-1' }); + vi.mocked(buildCliSpawnConfig).mockReturnValue({ command: 'claude', args: [] }); +} + beforeEach(() => { vi.clearAllMocks(); spawningTasks.clear(); runnerAgents.clear(); + setUseRunner(false); + vi.mocked(isTuiProvider).mockReturnValue(true); + // A runner spawn that resolves normally. Set for EVERY case, including the + // ones that must never reach the runner: a rejecting stub would fail those on + // a TypeError instead of on the assertion that names the invariant. + vi.mocked(spawnAgentViaRunner).mockResolvedValue({ pid: 4242 }); + // Truthy: a falsy in_progress write is read as "the claim did not land" and + // aborts the spawn before the dispatch. + vi.mocked(updateTask).mockResolvedValue({ metadata: {} }); vi.mocked(resolveAgentProviderAndModel).mockResolvedValue({ ok: true, provider: CLAUDE_TUI, selectedModel: 'sonnet', modelSelection: {}, }); }); +afterAll(() => rmSync(TEMP_ROOT, { recursive: true, force: true })); + describe('spawn setup failure after the worktree exists', () => { // Every failed Stage 2 spawn used to leave its checkout behind: the task // stayed pending and each retry cut another worktree. @@ -211,6 +287,10 @@ describe('public-review posture gate — spawn behavior (#5866)', () => { // be handed to the interactive PTY spawner. it('spawns a public-review stage on a TUI provider headless, never as a PTY session', async () => { const { buildTuiSpawnConfig, spawnTuiAgent } = await import('./agentTuiSpawning.js'); + // Without this the spawn stops at `prepareAgentWorkspace` and the negatives + // below hold for the wrong reason — no spawner is reached at all. + reachDispatch(); + await spawnAgentForTask({ id: 'task-public-review-tui', metadata: { @@ -220,7 +300,60 @@ describe('public-review posture gate — spawn behavior (#5866)', () => { }); expect(postureBlockWrites()).toEqual([]); + expect(spawnDirectly).toHaveBeenCalledTimes(1); expect(buildTuiSpawnConfig).not.toHaveBeenCalled(); expect(spawnTuiAgent).not.toHaveBeenCalled(); }); }); + +/** + * `const dispatchUseRunner = publicReview ? false : useRunner;` + * + * Direct-only is not a style choice: the two halves of a vendor's `no-tool` + * posture are enforced in different places. The argv is declared on the vendor + * row and resolved inside the spawn-config builder, so it survives any dispatch + * path — but OpenCode's posture lives entirely in the config content that + * `buildCliChildEnv` writes from the `safetyProfile` it is handed, and the CoS + * runner payload carries no `safetyProfile`. A public-review stage that reached + * the runner would therefore still LOOK enforced (right flag, right provider, + * gate passed) while running tool-enabled against contributor-authored PR text. + * Silent, and green in CI — so the routing itself is what gets pinned. + */ +describe('public-review dispatch — direct-only, never the CoS runner (#6105)', () => { + it('sends a public-review stage to the direct spawner even with runner mode on', async () => { + setUseRunner(true); + reachDispatch(); + + await spawnAgentForTask({ + id: 'task-public-review-runner', + metadata: { + executionProfile: 'public-review-gate', + pipeline: { securityScan: { completed: true, status: 'passed', safePrCount: 1 } }, + }, + }); + + expect(spawnAgentViaRunner).not.toHaveBeenCalled(); + expect(spawnDirectly).toHaveBeenCalledTimes(1); + // The posture the direct spawner hands to `buildCliChildEnv` — the half of + // the enforcement the runner payload has no field for. + expect(vi.mocked(spawnDirectly).mock.calls[0][0]).toMatchObject({ safetyProfile: 'public-review-gate' }); + }); + + // The control. Without it the assertion above would also pass if runner mode + // simply never reached the dispatch in this mock set, which is how the sibling + // TUI negatives came to hold vacuously. + it('still sends an ordinary task to the runner when runner mode is on', async () => { + setUseRunner(true); + // A non-TUI record: an ordinary TUI task is spawned as a PTY session, and + // the runner arm sits on the headless side of that branch. + vi.mocked(isTuiProvider).mockReturnValue(false); + reachDispatch(); + + await spawnAgentForTask({ id: 'task-ordinary-runner', metadata: {} }); + + expect(spawnAgentViaRunner).toHaveBeenCalledTimes(1); + expect(spawnDirectly).not.toHaveBeenCalled(); + // spawnViaRunner arms a 3s "still initializing" timer on the live agent. + for (const agent of runnerAgents.values()) clearTimeout(agent.initializationTimeout); + }); +});