From 36a0cfe39f6743b72c6bf56840a3f79ade720f09 Mon Sep 17 00:00:00 2001 From: Test User Date: Mon, 14 Sep 2026 08:59:27 -0500 Subject: [PATCH 1/3] fix(workflow): retryable TESTED SKIPPED verdicts and non-dead-end test_runner scope advice (#2756) - test_runner guard-2 message no longer recommends the scope its sibling guard blocks; it directs to files/targets with a concrete example - delegation-gate Stage B settlement treats a TESTED SKIPPED verdict as tests-not-run: no stage_b_failed, state stays Stage B eligible, reviewer proof preserved (architect re-dispatches the test gate) - background ingestBackgroundStageBCompletion classifies SKIPPED as 'skip' (consumed, skipped flag, no transition, proof preserved); completion-observer publishes a dedicated skip advisory instead of the generic 'ingestion failed' - genuine FAIL and REVIEWED rejections keep stage_b_failed semantics --- .../pending/2756-skipped-verdict-stage-b.md | 20 ++ src/background/completion-observer.ts | 16 +- src/background/stage-b-gates.ts | 24 +- src/hooks/delegation-gate.ts | 15 ++ src/tools/test-runner.ts | 2 +- .../stage-b-gates-skipped-verdict.test.ts | 200 +++++++++++++++ .../delegation-gate-stage-b-skipped.test.ts | 238 ++++++++++++++++++ .../tools/test-runner-scope-advice.test.ts | 61 +++++ 8 files changed, 566 insertions(+), 10 deletions(-) create mode 100644 docs/releases/pending/2756-skipped-verdict-stage-b.md create mode 100644 tests/unit/background/stage-b-gates-skipped-verdict.test.ts create mode 100644 tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts create mode 100644 tests/unit/tools/test-runner-scope-advice.test.ts diff --git a/docs/releases/pending/2756-skipped-verdict-stage-b.md b/docs/releases/pending/2756-skipped-verdict-stage-b.md new file mode 100644 index 000000000..e3e1d33d2 --- /dev/null +++ b/docs/releases/pending/2756-skipped-verdict-stage-b.md @@ -0,0 +1,20 @@ +# Stop test_runner's dead-end scope advice and score TESTED SKIPPED as retryable, not failed + +Issue: #2756 + +## What changed + +- **`test_runner` remediation text** (`src/tools/test-runner.ts`): the guard that rejects `scope:"convention"`/`"graph"`/`"impact"` without `files`/`targets` no longer recommends `scope:"all"` — the exact scope its sibling guard blocks for agent use (env-gated `SWARM_ALLOW_FULL_SUITE`) and the `test_engineer` prompt prohibits. The `message` field now directs the caller to pass a non-empty `files` array (or `targets` for framework-native names) with a concrete example. The `error` field text and both guards' semantics are unchanged. +- **Foreground Stage B verdict settlement** (`src/hooks/delegation-gate.ts`): a `[TESTED] | task-N | SKIPPED | ...` structured verdict (tests were NOT run — prohibited scope, framework detection none, missing test file) no longer emits `stage_b_failed`. The task stays in its Stage B eligible state (`reviewer_run`/`pre_check_passed`), the reviewer's APPROVED gate proof is preserved, and the `stageBCompletion` entry is untouched, so the architect can re-dispatch the test gate instead of forcing a coder rework of correct code. A warn log records the skip for the orchestrator. Genuine `FAIL` verdicts and `REVIEWED` rejections keep the existing `stage_b_failed` → `rework_required` semantics. +- **Background Stage B ingestion** (`src/background/stage-b-gates.ts`): `structuredStageBVerdict` now returns `'skip'` for TESTED SKIPPED; `ingestBackgroundStageBCompletion` consumes the record with the new `skipped: true` result flag and fires no transition and no proof clearing. `StageBIngestionResult` gains the optional `skipped` field. +- **Background advisory** (`src/background/completion-observer.ts`): a skipped ingestion publishes `skipped (tests not run) — re-dispatch the test gate; reviewer proof preserved; task remains Stage B eligible` instead of the generic `ingestion failed`, so operators can distinguish a retryable skip from a hard failure. + +## Why + +An agent that followed the tool's own advice (`scope:"all"`) could not succeed — the recommended scope is blocked — and models without a natural `files:` habit (observed: Kim K2.7 Code, 10–11 identical calls) looped until the repetition breaker fired. The prompt's SKIP CONDITION 1 then legitimately produced a `[TESTED] ... SKIPPED` verdict, which the gate scored as a code failure: `rework_required` plus deletion of the reviewer's approval for code that was correct and passing (`python -m pytest` green). Together with #2755 (no autonomous exit from `rework_required`, fixed by PR #2760's audited recovery tool), a single tool-argument mistake stranded tasks that only a human could free. This fix removes the wrongful entry: tests-not-run is retryable state, not failure. + +## Tests + +- `tests/unit/tools/test-runner-scope-advice.test.ts` — guard-2 message directs to files/targets and never recommends the blocked scope (all three guarded scopes); guard-1 block and the pinned `error` text are characterized as unchanged. +- `tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts` — SKIPPED leaves state `reviewer_run` with durable reviewer proof and the reviewer completion entry intact; FAIL and REVIEWED REJECTED still go `rework_required` with proof cleared. +- `tests/unit/background/stage-b-gates-skipped-verdict.test.ts` — SKIPPED ingest returns `skipped: true`, no state mutation, proof preserved; FAIL and unparseable output keep the fail-closed rejection. diff --git a/src/background/completion-observer.ts b/src/background/completion-observer.ts index 372f0c585..2be6b155f 100644 --- a/src/background/completion-observer.ts +++ b/src/background/completion-observer.ts @@ -523,13 +523,15 @@ export function createBackgroundCompletionObserver(opts: { directory, record, terminal.eventId, - legacyTransferPending - ? 'ingestion failed; legacy coder settlement transfer is pending; durable reconciliation will retry' - : legacyTransferRequiresManualRecovery - ? 'ingestion failed; legacy coder settlement requires manual recovery; run /swarm recover for this task (or /swarm reset-session)' - : applied.stale - ? 'stale' - : 'ingestion failed', + applied.skipped + ? 'skipped (tests not run) — re-dispatch the test gate; reviewer proof preserved; task remains Stage B eligible' + : legacyTransferPending + ? 'ingestion failed; legacy coder settlement transfer is pending; durable reconciliation will retry' + : legacyTransferRequiresManualRecovery + ? 'ingestion failed; legacy coder settlement requires manual recovery; run /swarm recover for this task (or /swarm reset-session)' + : applied.stale + ? 'stale' + : 'ingestion failed', ); // Maintenance point P2b (issue #2104): the ingestion // rejection has just been durably recorded — reconcile now diff --git a/src/background/stage-b-gates.ts b/src/background/stage-b-gates.ts index 6e4f38968..0cd3c1a12 100644 --- a/src/background/stage-b-gates.ts +++ b/src/background/stage-b-gates.ts @@ -83,7 +83,7 @@ function structuredStageBVerdict( role: StageBStateRole, text: string, taskId: string, -): 'pass' | 'fail' | null { +): 'pass' | 'fail' | 'skip' | null { const escapedTaskId = taskId.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); const pattern = role === 'reviewer' @@ -97,7 +97,11 @@ function structuredStageBVerdict( ); const match = pattern.exec(text); if (!match) return null; - return match[1] === 'APPROVED' || match[1] === 'PASS' ? 'pass' : 'fail'; + if (match[1] === 'APPROVED' || match[1] === 'PASS') return 'pass'; + // SKIPPED means the tests were not run (issue #2756): a tool-argument + // outcome the caller can retry, not a code failure. + if (match[1] === 'SKIPPED') return 'skip'; + return 'fail'; } function normalizeAttributionPath(file: string): string | null { @@ -218,6 +222,10 @@ export interface StageBIngestionResult { ok: boolean; consumed: boolean; stale?: boolean; + /** True when the gate reported a not-run outcome (TESTED SKIPPED): no + * transition fired, no proof was cleared, and the task remains Stage B + * eligible for re-dispatch (issue #2756). */ + skipped?: boolean; reason?: string; } @@ -624,6 +632,18 @@ export async function ingestBackgroundStageBCompletion(args: { const verdict = stageBRole ? structuredStageBVerdict(stageBRole, args.result.text ?? '', taskId) : null; + if (stageBRole && verdict === 'skip') { + // TESTED SKIPPED = tests not run (issue #2756): consume the record so + // the observer publishes the skip advisory and no retry loop forms, + // but fire NO stage_b_failed transition and clear no gate proof — + // the task stays Stage B eligible for a test-gate re-dispatch. + return { + ok: false, + consumed: true, + skipped: true, + reason: `background ${stageBRole} skipped task ${taskId} — tests not run; re-dispatch the test gate (task stays Stage B eligible; reviewer proof preserved)`, + }; + } if (stageBRole && verdict !== 'pass') { const rejected = await transitionTaskWorkflowEvidence( args.directory, diff --git a/src/hooks/delegation-gate.ts b/src/hooks/delegation-gate.ts index 655ec8f57..ba7790e2a 100644 --- a/src/hooks/delegation-gate.ts +++ b/src/hooks/delegation-gate.ts @@ -5948,6 +5948,21 @@ export function createDelegationGateHook( const verdictEntry = attributionResult.verdicts.get(taskId); const dispatchCtxForVerdict = stageBDispatchContextByCallID.get(input.callID); + // A SKIPPED TESTED verdict means the tests were not run + // (e.g. prohibited scope, framework detection none) — a + // tool-argument outcome, not a code failure. Leave the + // task in its Stage B eligible state with the reviewer + // proof intact so the architect can re-dispatch the test + // gate instead of forcing a coder rework (issue #2756). + if ( + dispatchCtxForVerdict?.expectedVerdictKind === 'TESTED' && + verdictEntry?.verdict === 'SKIPPED' + ) { + logger.warn( + `[delegation-gate] Stage B test gate SKIPPED (tests not run) for task ${taskId} from call ${input.callID} — leaving state ${state} for test-gate re-dispatch; reviewer proof preserved`, + ); + continue; + } const positiveVerdict = dispatchCtxForVerdict?.expectedVerdictKind === 'TESTED' ? verdictEntry?.verdict === 'PASS' diff --git a/src/tools/test-runner.ts b/src/tools/test-runner.ts index 7738ddf07..47b6509ca 100644 --- a/src/tools/test-runner.ts +++ b/src/tools/test-runner.ts @@ -3166,7 +3166,7 @@ export const test_runner: ReturnType = createSwarmTool({ error: 'scope "convention" and "graph" require explicit files or targets array - omitting both causes unsafe full-project discovery', message: - 'When using scope "convention" or "graph", you must provide a non-empty "files" or "targets" array. Use scope "all" for full project test suite without specifying files.', + 'When using scope "convention", "graph", or "impact", you must provide a non-empty "files" array (or "targets" for framework-native test names). Example: { scope: "convention", files: ["tests/test_calc.py"] }', outcome: 'error', resolution: makeResolution(scope, scope, [], [], 'skip', workingDir), }; diff --git a/tests/unit/background/stage-b-gates-skipped-verdict.test.ts b/tests/unit/background/stage-b-gates-skipped-verdict.test.ts new file mode 100644 index 000000000..bc7dec141 --- /dev/null +++ b/tests/unit/background/stage-b-gates-skipped-verdict.test.ts @@ -0,0 +1,200 @@ +/** + * Issue #2756 regression tests — background Stage B TESTED SKIPPED verdict. + * + * `ingestBackgroundStageBCompletion` must classify a `[TESTED] | task-N | + * SKIPPED | ...` structured verdict as a not-run skip (issue #2756 defect 2, + * background path): no stage_b_failed transition, no rework_required, reviewer + * gate proof preserved, and the record consumed with `skipped: true` so the + * completion observer publishes the dedicated skip advisory. Genuine FAIL + * verdicts keep the rejection semantics. + */ + +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import type { + BackgroundDelegationRecord, + BackgroundWorkspaceSnapshot, +} from '../../../src/background/pending-delegations.js'; +import { ingestBackgroundStageBCompletion } from '../../../src/background/stage-b-gates.js'; +import { captureWorkspaceSnapshot } from '../../../src/background/workspace-snapshot.js'; +import { + readTaskEvidence, + recordGateEvidence, + transitionTaskWorkflowEvidence, +} from '../../../src/gate-evidence.js'; +import { + resetSwarmState, + startAgentSession, + swarmState, +} from '../../../src/state.js'; +import { createIsolatedTestEnv } from '../../helpers/isolated-test-env.js'; +import { canonicalMkdtemp } from '../../helpers/tmpdir.js'; + +const TASK_ID = '1.1'; + +let directory = ''; +let isolatedEnv: ReturnType | undefined; + +function git(...args: string[]): void { + const result = spawnSync('git', args, { + cwd: directory, + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + timeout: 5_000, + maxBuffer: 64 * 1024, + windowsHide: true, + }); + if (result.status !== 0) throw new Error(result.stderr || result.stdout); +} + +function stageBRecord( + workspace: BackgroundWorkspaceSnapshot, +): BackgroundDelegationRecord { + return { + schemaVersion: 2, + correlationId: 'call-2756-skip:correlation', + jobId: 'call-2756-skip:job', + subagentSessionId: 'child-2756-skip', + parentSessionId: 'parent-2756-skip', + callID: 'call-2756-skip', + normalizedAgent: 'test_engineer', + swarmPrefixedAgent: 'test_engineer', + planTaskId: TASK_ID, + evidenceTaskId: TASK_ID, + status: 'completed', + createdAt: 1, + updatedAt: 2, + completedAt: 2, + workflowGeneration: 1, + workspace, + }; +} + +/** accepted_mutation (gen 0→1) → stage_a_passed → reviewer proof; state reviewer_run. */ +async function prepareTask(): Promise { + await transitionTaskWorkflowEvidence(directory, TASK_ID, { + type: 'accepted_mutation', + agentType: 'coder', + expectedGeneration: 0, + transitionId: `coder:${TASK_ID}`, + }); + await transitionTaskWorkflowEvidence(directory, TASK_ID, { + type: 'stage_a_passed', + expectedGeneration: 1, + transitionId: `stage-a:${TASK_ID}`, + }); + await recordGateEvidence( + directory, + TASK_ID, + 'reviewer', + 'seed-reviewer', + undefined, + { + expectedGeneration: 1, + transitionId: `seed-reviewer:${TASK_ID}`, + }, + ); + const session = swarmState.agentSessions.get('parent-2756-skip')!; + session.taskWorkflowStates.set(TASK_ID, 'reviewer_run'); +} + +async function ingest(text: string) { + return ingestBackgroundStageBCompletion({ + directory, + record: stageBRecord(captureWorkspaceSnapshot(directory)), + result: { + text, + chars: text.length, + truncated: false, + digest: 'call-2756-skip:digest', + }, + }); +} + +beforeEach(() => { + isolatedEnv = createIsolatedTestEnv(); + resetSwarmState(); + directory = canonicalMkdtemp('bg-stage-b-skipped-'); + fs.mkdirSync(path.join(directory, '.opencode'), { recursive: true }); + fs.mkdirSync(path.join(directory, '.swarm'), { recursive: true }); + git('init'); + git('config', 'user.email', 'tests@example.com'); + git('config', 'user.name', 'Tests'); + fs.writeFileSync(path.join(directory, 'base.txt'), 'base\n'); + git('add', 'base.txt'); + git('commit', '-m', 'test: issue 2756 fixture'); + startAgentSession('parent-2756-skip', 'architect', directory); +}); + +afterEach(() => { + resetSwarmState(); + try { + fs.rmSync(directory, { + recursive: true, + force: true, + maxRetries: 5, + retryDelay: 100, + }); + } catch { + // best-effort cleanup + } + isolatedEnv?.cleanup(); + isolatedEnv = undefined; +}); + +describe('background Stage B TESTED SKIPPED verdict is retryable, not a failure (#2756)', () => { + test('SKIPPED ingest: no rejection, no state mutation, reviewer proof preserved, skipped flag set', async () => { + await prepareTask(); + + const outcome = await ingest( + `[TESTED] | task-${TASK_ID} | SKIPPED | PROHIBITED SCOPE: test_runner refuses scope "all" — tests not run`, + ); + + expect(outcome.skipped).toBe(true); + expect(outcome.consumed).toBe(true); + expect(outcome.ok).toBe(false); + expect(outcome.reason ?? '').not.toMatch(/rejected task/); + + const session = swarmState.agentSessions.get('parent-2756-skip')!; + expect(session.taskWorkflowStates.get(TASK_ID)).toBe('reviewer_run'); + + const evidence = await readTaskEvidence(directory, TASK_ID); + expect(evidence?.workflow?.state).toBe('reviewer_run'); + expect(evidence?.workflow?.lastOutcome).not.toBe('stage_b_failed'); + expect(evidence?.gates?.reviewer).toBeDefined(); + }); + + test('genuine FAIL ingest keeps the rejection semantics', async () => { + await prepareTask(); + + const outcome = await ingest( + `[TESTED] | task-${TASK_ID} | FAIL | 6/10 tests passed - missing error path tests`, + ); + + expect(outcome.skipped).toBeUndefined(); + expect(outcome.consumed).toBe(true); + expect(outcome.ok).toBe(false); + expect(outcome.reason ?? '').toMatch(/rejected task/); + + const session = swarmState.agentSessions.get('parent-2756-skip')!; + expect(session.taskWorkflowStates.get(TASK_ID)).toBe('rework_required'); + + const evidence = await readTaskEvidence(directory, TASK_ID); + expect(evidence?.workflow?.state).toBe('rework_required'); + expect(evidence?.workflow?.lastOutcome).toBe('stage_b_failed'); + expect(evidence?.gates?.reviewer).toBeUndefined(); + }); + + test('unparseable output (no structured verdict line) keeps the fail-closed rejection', async () => { + await prepareTask(); + + const outcome = await ingest('VERDICT: unclear, no structured line'); + + expect(outcome.skipped).toBeUndefined(); + expect(outcome.ok).toBe(false); + const session = swarmState.agentSessions.get('parent-2756-skip')!; + expect(session.taskWorkflowStates.get(TASK_ID)).toBe('rework_required'); + }); +}); diff --git a/tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts b/tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts new file mode 100644 index 000000000..2f91863d9 --- /dev/null +++ b/tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts @@ -0,0 +1,238 @@ +/** + * Issue #2756 regression tests — foreground Stage B TESTED SKIPPED verdict. + * + * A `[TESTED] | task-N | SKIPPED | ...` verdict means the tests were NOT run + * (tool-argument outcome, e.g. prohibited scope / framework detection none). + * The delegation gate must leave the task in its Stage B eligible state with + * the reviewer's gate proof intact so the architect can re-dispatch the test + * gate — NOT score it as stage_b_failed/rework_required (issue #2756 defect 2, + * foreground path). Genuine FAIL verdicts keep the rejection semantics. + */ + +import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { + readTaskEvidence, + recordAgentDispatch, + recordGateEvidence, + transitionTaskWorkflowEvidence, +} from '../../../src/gate-evidence'; +import { createDelegationGateHook } from '../../../src/hooks/delegation-gate'; +import { + ensureAgentSession, + resetSwarmState, + startAgentSession, +} from '../../../src/state'; +import { createIsolatedTestEnv } from '../../helpers/isolated-test-env.js'; +import { canonicalMkdtemp } from '../../helpers/tmpdir.js'; + +let tempDir: string; +let isolatedEnv: ReturnType | undefined; + +function makeConfig() { + return { + max_iterations: 5, + qa_retry_limit: 3, + inject_phase_reminders: true, + hooks: { + system_enhancer: true, + compaction: true, + agent_activity: true, + delegation_tracker: false, + agent_awareness_max_chars: 300, + delegation_gate: true, + delegation_max_chars: 4000, + }, + } as import('../../../src/config').PluginConfig; +} + +function writePlan(directory: string, taskIds: string[]): void { + fs.writeFileSync( + path.join(directory, '.swarm', 'plan.json'), + JSON.stringify({ + schema_version: '1.0.0', + title: 'Stage B skipped verdict test', + swarm: 'test', + current_phase: 1, + phases: [ + { + id: 1, + name: 'Implementation', + status: 'in_progress', + tasks: taskIds.map((id) => ({ + id, + phase: 1, + status: 'in_progress', + size: 'small', + description: `Implement ${id}`, + depends: [], + files_touched: [], + })), + }, + ], + }), + ); +} + +/** Stage A passed + reviewer APPROVED durable proof; task state reviewer_run. */ +async function seedReviewerApproved( + directory: string, + taskId: string, +): Promise { + writePlan(directory, [taskId]); + await recordAgentDispatch(directory, taskId, 'coder'); + const generation = (await readTaskEvidence(directory, taskId))!.workflow! + .generation; + await transitionTaskWorkflowEvidence(directory, taskId, { + type: 'stage_a_passed', + expectedGeneration: generation, + }); + await recordGateEvidence( + directory, + taskId, + 'reviewer', + 'seed-reviewer', + undefined, + { + expectedGeneration: generation, + }, + ); +} + +async function runTestEngineerDispatch( + hook: ReturnType, + sessionID: string, + callID: string, + taskId: string, + verdictLine: string, +): Promise { + const args = { + subagent_type: 'test_engineer', + task_id: taskId, + prompt: `TASK: ${taskId}\nTASKS: ${taskId}\nACCEPTANCE: test_engineer must report an exact structured verdict for every listed task`, + }; + await hook.toolBefore({ tool: 'Task', sessionID, callID }, { args }); + await hook.toolAfter( + { tool: 'Task', sessionID, callID, args }, + { output: verdictLine }, + ); +} + +beforeEach(() => { + isolatedEnv = createIsolatedTestEnv(); + resetSwarmState(); + tempDir = canonicalMkdtemp('dg-stage-b-skipped-'); + fs.mkdirSync(path.join(tempDir, '.opencode'), { recursive: true }); + fs.mkdirSync(path.join(tempDir, '.swarm'), { recursive: true }); +}); + +afterEach(() => { + resetSwarmState(); + try { + fs.rmSync(tempDir, { + recursive: true, + force: true, + maxRetries: 5, + retryDelay: 100, + }); + } catch { + // best-effort cleanup + } + isolatedEnv?.cleanup(); + isolatedEnv = undefined; +}); + +describe('Stage B TESTED SKIPPED verdict is retryable, not a failure (#2756)', () => { + it('SKIPPED leaves the task at reviewer_run with reviewer proof intact', async () => { + const hook = createDelegationGateHook(makeConfig(), tempDir); + startAgentSession('sess-skip-1', 'architect'); + const session = ensureAgentSession('sess-skip-1'); + await seedReviewerApproved(tempDir, '1.1'); + session.taskWorkflowStates.set('1.1', 'reviewer_run'); + session.currentTaskId = '1.1'; + + await runTestEngineerDispatch( + hook, + 'sess-skip-1', + 'call-skip-1', + '1.1', + '[TESTED] | task-1.1 | SKIPPED | PROHIBITED SCOPE: test_runner refuses scope "all" — tests not run', + ); + + expect(session.taskWorkflowStates.get('1.1')).toBe('reviewer_run'); + const evidence = await readTaskEvidence(tempDir, '1.1'); + expect(evidence?.workflow?.state).toBe('reviewer_run'); + expect(evidence?.workflow?.lastOutcome).not.toBe('stage_b_failed'); + expect(evidence?.gates?.reviewer).toBeDefined(); + }); + + it('SKIPPED does not consume the reviewer completion entry', async () => { + const hook = createDelegationGateHook(makeConfig(), tempDir); + startAgentSession('sess-skip-2', 'architect'); + const session = ensureAgentSession('sess-skip-2'); + await seedReviewerApproved(tempDir, '1.1'); + session.taskWorkflowStates.set('1.1', 'reviewer_run'); + session.currentTaskId = '1.1'; + session.stageBCompletion?.set('1.1', new Set(['reviewer'])); + + await runTestEngineerDispatch( + hook, + 'sess-skip-2', + 'call-skip-2', + '1.1', + '[TESTED] | task-1.1 | SKIPPED | framework detection returned none', + ); + + expect(session.taskWorkflowStates.get('1.1')).toBe('reviewer_run'); + expect(session.stageBCompletion?.get('1.1')).toBeDefined(); + }); + + it('genuine FAIL verdict still moves the task to rework_required and clears reviewer proof', async () => { + const hook = createDelegationGateHook(makeConfig(), tempDir); + startAgentSession('sess-fail-1', 'architect'); + const session = ensureAgentSession('sess-fail-1'); + await seedReviewerApproved(tempDir, '1.1'); + session.taskWorkflowStates.set('1.1', 'reviewer_run'); + session.currentTaskId = '1.1'; + + await runTestEngineerDispatch( + hook, + 'sess-fail-1', + 'call-fail-1', + '1.1', + '[TESTED] | task-1.1 | FAIL | 6/10 tests passed — missing error path tests', + ); + + expect(session.taskWorkflowStates.get('1.1')).toBe('rework_required'); + const evidence = await readTaskEvidence(tempDir, '1.1'); + expect(evidence?.workflow?.state).toBe('rework_required'); + expect(evidence?.gates?.reviewer).toBeUndefined(); + }); + + it('REVIEWED REJECTED still moves the task to rework_required', async () => { + const hook = createDelegationGateHook(makeConfig(), tempDir); + startAgentSession('sess-rej-1', 'architect'); + const session = ensureAgentSession('sess-rej-1'); + await seedReviewerApproved(tempDir, '1.1'); + session.taskWorkflowStates.set('1.1', 'reviewer_run'); + session.currentTaskId = '1.1'; + + const args = { + subagent_type: 'reviewer', + task_id: '1.1', + prompt: + 'TASK: 1.1\nTASKS: 1.1\nACCEPTANCE: reviewer must report an exact structured verdict for every listed task', + }; + await hook.toolBefore( + { tool: 'Task', sessionID: 'sess-rej-1', callID: 'call-rej-1' }, + { args }, + ); + await hook.toolAfter( + { tool: 'Task', sessionID: 'sess-rej-1', callID: 'call-rej-1', args }, + { output: '[REVIEWED] | task-1.1 | REJECTED | critical defect found' }, + ); + + expect(session.taskWorkflowStates.get('1.1')).toBe('rework_required'); + }); +}); diff --git a/tests/unit/tools/test-runner-scope-advice.test.ts b/tests/unit/tools/test-runner-scope-advice.test.ts new file mode 100644 index 000000000..d587655d3 --- /dev/null +++ b/tests/unit/tools/test-runner-scope-advice.test.ts @@ -0,0 +1,61 @@ +/** + * Issue #2756 regression tests — test_runner scope-guard remediation advice. + * + * The guard that rejects scope "convention"/"graph"/"impact" without + * files/targets must direct the caller to provide files/targets and must NOT + * recommend scope "all": that scope is blocked for agent use by the sibling + * guard (SWARM_ALLOW_FULL_SUITE env-gated), so recommending it sends agents + * into an unrecoverable loop (issue #2756 defect 1). + */ + +import { beforeAll, describe, expect, test } from 'bun:test'; +import { test_runner } from '../../../src/tools/test-runner'; + +beforeAll(() => { + // Guard 1 opt-in must be absent so the sibling full-suite guard stays live; + // the recommended workaround would be a proven dead end. + delete process.env.SWARM_ALLOW_FULL_SUITE; +}); + +describe('test_runner missing-files guard remediation advice (#2756)', () => { + test('convention-without-files message directs to files/targets and never recommends scope "all"', async () => { + const result = await test_runner.execute( + { scope: 'convention' }, + {} as any, + ); + const parsed = JSON.parse(result) as { + success?: boolean; + message?: string; + error?: string; + }; + expect(parsed.success).toBe(false); + expect(parsed.message ?? '').toMatch(/files|targets/i); + expect(parsed.message ?? '').not.toContain('scope "all"'); + expect(parsed.error ?? '').not.toContain('scope "all"'); + }); + + test('graph-without-files and impact-without-files return the same non-dead-end advice', async () => { + for (const scope of ['graph', 'impact'] as const) { + const result = await test_runner.execute({ scope }, {} as any); + const parsed = JSON.parse(result) as { message?: string }; + expect(parsed.message ?? '').toMatch(/files|targets/i); + expect(parsed.message ?? '').not.toContain('scope "all"'); + } + }); + + test('guard 1 still blocks scope "all" for agent use (characterization)', async () => { + const result = await test_runner.execute({ scope: 'all' }, {} as any); + const parsed = JSON.parse(result) as { success?: boolean; error?: string }; + expect(parsed.success).toBe(false); + expect(parsed.error ?? '').toContain('scope "all" is blocked'); + }); + + test('guard 2 still rejects empty files arrays with the pinned error text', async () => { + const result = await test_runner.execute( + { scope: 'convention', files: [] }, + {} as any, + ); + const parsed = JSON.parse(result) as { error?: string }; + expect(parsed.error ?? '').toContain('require explicit files'); + }); +}); From 021ba9b96273871adf62482cf8d2bdb1d17fd79f Mon Sep 17 00:00:00 2001 From: Test User Date: Mon, 14 Sep 2026 11:01:04 -0500 Subject: [PATCH 2/3] fix(review): address PR #2766 review findings (PRR-006..010) - PRR-006: observer-path test proving the dedicated skip advisory is published (and 'ingestion failed' is not) via the full completion observer flow; test docstring now states only what is asserted - PRR-007: guard-2 error field enumerates all three covered scopes; enumeration pinned in tests for both response fields - PRR-008: release fragment model name typo (Kimi K2.7 Code) - PRR-009: SWARM_ALLOW_FULL_SUITE saved/restored around the suite - PRR-010: docs/planning.md rework_required entry condition notes the TESTED SKIPPED exception --- docs/planning.md | 2 +- .../pending/2756-skipped-verdict-stage-b.md | 2 +- src/tools/test-runner.ts | 2 +- .../stage-b-gates-skipped-verdict.test.ts | 77 +++++++++++++++++-- .../tools/test-runner-scope-advice.test.ts | 31 +++++++- 5 files changed, 102 insertions(+), 12 deletions(-) diff --git a/docs/planning.md b/docs/planning.md index 031166d46..3dde73aa6 100644 --- a/docs/planning.md +++ b/docs/planning.md @@ -89,7 +89,7 @@ Each task in the swarm follows a per-task state machine. The Architect advances | `pre_check_passed` | Automated gates passed | `pre_check_batch` returns `gates_passed: true` | | `reviewer_run` | Human-style review complete | Reviewer delegation returns APPROVED | | `tests_run` | Verification tests passed | Test engineer delegation returns PASS | -| `rework_required` | Current-generation verification failed and same-task repair is required | Stage A fails, or reviewer/test engineer returns a negative or malformed verdict | +| `rework_required` | Current-generation verification failed and same-task repair is required | Stage A fails, or reviewer/test engineer returns a negative or malformed verdict. A TESTED `SKIPPED` verdict (tests not run) is the exception: it stays Stage B eligible for test-gate re-dispatch instead of entering `rework_required` (#2756) | | `blocked` | Task ended without completion and no verification debt remains | `update_task_status(status: 'blocked')` commits the terminal transaction | | `closed` | A session ended with unfinished work; this is not successful completion | `/swarm close` commits a plan-bound `task_closed` transition | | `complete` | Task fully complete | `update_task_status(status: 'completed')` called | diff --git a/docs/releases/pending/2756-skipped-verdict-stage-b.md b/docs/releases/pending/2756-skipped-verdict-stage-b.md index e3e1d33d2..e02837575 100644 --- a/docs/releases/pending/2756-skipped-verdict-stage-b.md +++ b/docs/releases/pending/2756-skipped-verdict-stage-b.md @@ -11,7 +11,7 @@ Issue: #2756 ## Why -An agent that followed the tool's own advice (`scope:"all"`) could not succeed — the recommended scope is blocked — and models without a natural `files:` habit (observed: Kim K2.7 Code, 10–11 identical calls) looped until the repetition breaker fired. The prompt's SKIP CONDITION 1 then legitimately produced a `[TESTED] ... SKIPPED` verdict, which the gate scored as a code failure: `rework_required` plus deletion of the reviewer's approval for code that was correct and passing (`python -m pytest` green). Together with #2755 (no autonomous exit from `rework_required`, fixed by PR #2760's audited recovery tool), a single tool-argument mistake stranded tasks that only a human could free. This fix removes the wrongful entry: tests-not-run is retryable state, not failure. +An agent that followed the tool's own advice (`scope:"all"`) could not succeed — the recommended scope is blocked — and models without a natural `files:` habit (observed: Kimi K2.7 Code, 10–11 identical calls) looped until the repetition breaker fired. The prompt's SKIP CONDITION 1 then legitimately produced a `[TESTED] ... SKIPPED` verdict, which the gate scored as a code failure: `rework_required` plus deletion of the reviewer's approval for code that was correct and passing (`python -m pytest` green). Together with #2755 (no autonomous exit from `rework_required`, fixed by PR #2760's audited recovery tool), a single tool-argument mistake stranded tasks that only a human could free. This fix removes the wrongful entry: tests-not-run is retryable state, not failure. ## Tests diff --git a/src/tools/test-runner.ts b/src/tools/test-runner.ts index 47b6509ca..dc50956c3 100644 --- a/src/tools/test-runner.ts +++ b/src/tools/test-runner.ts @@ -3164,7 +3164,7 @@ export const test_runner: ReturnType = createSwarmTool({ framework: 'none', scope, error: - 'scope "convention" and "graph" require explicit files or targets array - omitting both causes unsafe full-project discovery', + 'scope "convention", "graph", and "impact" require explicit files or targets array - omitting both causes unsafe full-project discovery', message: 'When using scope "convention", "graph", or "impact", you must provide a non-empty "files" array (or "targets" for framework-native test names). Example: { scope: "convention", files: ["tests/test_calc.py"] }', outcome: 'error', diff --git a/tests/unit/background/stage-b-gates-skipped-verdict.test.ts b/tests/unit/background/stage-b-gates-skipped-verdict.test.ts index bc7dec141..9e06d21a4 100644 --- a/tests/unit/background/stage-b-gates-skipped-verdict.test.ts +++ b/tests/unit/background/stage-b-gates-skipped-verdict.test.ts @@ -4,8 +4,9 @@ * `ingestBackgroundStageBCompletion` must classify a `[TESTED] | task-N | * SKIPPED | ...` structured verdict as a not-run skip (issue #2756 defect 2, * background path): no stage_b_failed transition, no rework_required, reviewer - * gate proof preserved, and the record consumed with `skipped: true` so the - * completion observer publishes the dedicated skip advisory. Genuine FAIL + * gate proof preserved, and the record consumed with `skipped: true`. The + * observer-path test additionally proves the completion observer publishes the + * dedicated skip advisory (not the generic "ingestion failed"). Genuine FAIL * verdicts keep the rejection semantics. */ @@ -13,9 +14,11 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as path from 'node:path'; -import type { - BackgroundDelegationRecord, - BackgroundWorkspaceSnapshot, +import { createBackgroundCompletionObserver } from '../../../src/background/completion-observer'; +import { + type BackgroundWorkspaceSnapshot, + findByCorrelationId, + recordPendingDelegation, } from '../../../src/background/pending-delegations.js'; import { ingestBackgroundStageBCompletion } from '../../../src/background/stage-b-gates.js'; import { captureWorkspaceSnapshot } from '../../../src/background/workspace-snapshot.js'; @@ -51,7 +54,7 @@ function git(...args: string[]): void { function stageBRecord( workspace: BackgroundWorkspaceSnapshot, -): BackgroundDelegationRecord { +): import('../../../src/background/pending-delegations.js').BackgroundDelegationRecord { return { schemaVersion: 2, correlationId: 'call-2756-skip:correlation', @@ -198,3 +201,65 @@ describe('background Stage B TESTED SKIPPED verdict is retryable, not a failure expect(session.taskWorkflowStates.get(TASK_ID)).toBe('rework_required'); }); }); + +describe('completion observer publishes the dedicated skip advisory (#2756, PRR-006)', () => { + const CORRELATION_ID = 'call-2756-skip-obs'; + const PARENT = 'parent-2756-skip'; + const SKIP_TEXT = `[TESTED] | task-${TASK_ID} | SKIPPED | PROHIBITED SCOPE: tests not run`; + + function completedEnvelope(): object { + return { + event: { + type: 'message.part.updated', + properties: { + part: { + type: 'text', + synthetic: true, + sessionID: PARENT, + text: `\n${SKIP_TEXT}\n\n`, + }, + }, + }, + }; + } + + test('observer path: skip advisory queued for the session, no stage_b_failed, record consumed', async () => { + await prepareTask(); + const session = swarmState.agentSessions.get(PARENT)!; + await recordPendingDelegation(directory, { + correlationId: CORRELATION_ID, + jobId: `${CORRELATION_ID}:job`, + subagentSessionId: CORRELATION_ID, + parentSessionId: PARENT, + callID: CORRELATION_ID, + normalizedAgent: 'test_engineer', + swarmPrefixedAgent: 'test_engineer', + planTaskId: TASK_ID, + evidenceTaskId: TASK_ID, + workflowGeneration: 1, + workspace: captureWorkspaceSnapshot(directory), + }); + + const observer = createBackgroundCompletionObserver({ + config: { enabled: true }, + directory, + }); + await observer.event(completedEnvelope()); + + const advisories = session.pendingAdvisoryMessages ?? []; + expect( + advisories.some((message) => message.includes('skipped (tests not run)')), + ).toBe(true); + expect( + advisories.some((message) => message.includes('ingestion failed')), + ).toBe(false); + + const record = findByCorrelationId(directory, CORRELATION_ID); + expect(record?.status).not.toBe('stale'); + + expect(session.taskWorkflowStates.get(TASK_ID)).toBe('reviewer_run'); + const evidence = await readTaskEvidence(directory, TASK_ID); + expect(evidence?.workflow?.lastOutcome).not.toBe('stage_b_failed'); + expect(evidence?.gates?.reviewer).toBeDefined(); + }); +}); diff --git a/tests/unit/tools/test-runner-scope-advice.test.ts b/tests/unit/tools/test-runner-scope-advice.test.ts index d587655d3..e08636e39 100644 --- a/tests/unit/tools/test-runner-scope-advice.test.ts +++ b/tests/unit/tools/test-runner-scope-advice.test.ts @@ -5,18 +5,31 @@ * files/targets must direct the caller to provide files/targets and must NOT * recommend scope "all": that scope is blocked for agent use by the sibling * guard (SWARM_ALLOW_FULL_SUITE env-gated), so recommending it sends agents - * into an unrecoverable loop (issue #2756 defect 1). + * into an unrecoverable loop (issue #2756 defect 1). Both response fields must + * enumerate every scope the guard actually covers. */ -import { beforeAll, describe, expect, test } from 'bun:test'; +import { afterAll, beforeAll, describe, expect, test } from 'bun:test'; import { test_runner } from '../../../src/tools/test-runner'; +let savedAllowFullSuite: string | undefined; + beforeAll(() => { // Guard 1 opt-in must be absent so the sibling full-suite guard stays live; - // the recommended workaround would be a proven dead end. + // the recommended workaround would be a proven dead end. Save and restore + // so the shard's env state is untouched (PRR-009). + savedAllowFullSuite = process.env.SWARM_ALLOW_FULL_SUITE; delete process.env.SWARM_ALLOW_FULL_SUITE; }); +afterAll(() => { + if (savedAllowFullSuite === undefined) { + delete process.env.SWARM_ALLOW_FULL_SUITE; + } else { + process.env.SWARM_ALLOW_FULL_SUITE = savedAllowFullSuite; + } +}); + describe('test_runner missing-files guard remediation advice (#2756)', () => { test('convention-without-files message directs to files/targets and never recommends scope "all"', async () => { const result = await test_runner.execute( @@ -43,6 +56,18 @@ describe('test_runner missing-files guard remediation advice (#2756)', () => { } }); + test('both response fields enumerate every scope the guard covers (PRR-007)', async () => { + for (const scope of ['convention', 'graph', 'impact'] as const) { + const result = await test_runner.execute({ scope }, {} as any); + const parsed = JSON.parse(result) as { + message?: string; + error?: string; + }; + expect(parsed.message ?? '').toContain('"impact"'); + expect(parsed.error ?? '').toContain('"impact"'); + } + }); + test('guard 1 still blocks scope "all" for agent use (characterization)', async () => { const result = await test_runner.execute({ scope: 'all' }, {} as any); const parsed = JSON.parse(result) as { success?: boolean; error?: string }; From bfc0e102aa8b71750ecacc06f2daa72df896c8c8 Mon Sep 17 00:00:00 2001 From: Test User Date: Mon, 14 Sep 2026 11:13:41 -0500 Subject: [PATCH 3/3] docs(review): correct stale error-field and test-count claims in fragment and PR body (critic round 1) --- docs/releases/pending/2756-skipped-verdict-stage-b.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/releases/pending/2756-skipped-verdict-stage-b.md b/docs/releases/pending/2756-skipped-verdict-stage-b.md index e02837575..e0927b127 100644 --- a/docs/releases/pending/2756-skipped-verdict-stage-b.md +++ b/docs/releases/pending/2756-skipped-verdict-stage-b.md @@ -4,7 +4,7 @@ Issue: #2756 ## What changed -- **`test_runner` remediation text** (`src/tools/test-runner.ts`): the guard that rejects `scope:"convention"`/`"graph"`/`"impact"` without `files`/`targets` no longer recommends `scope:"all"` — the exact scope its sibling guard blocks for agent use (env-gated `SWARM_ALLOW_FULL_SUITE`) and the `test_engineer` prompt prohibits. The `message` field now directs the caller to pass a non-empty `files` array (or `targets` for framework-native names) with a concrete example. The `error` field text and both guards' semantics are unchanged. +- **`test_runner` remediation text** (`src/tools/test-runner.ts`): the guard that rejects `scope:"convention"`/`"graph"`/`"impact"` without `files`/`targets` no longer recommends `scope:"all"` — the exact scope its sibling guard blocks for agent use (env-gated `SWARM_ALLOW_FULL_SUITE`) and the `test_engineer` prompt prohibits. The `message` field now directs the caller to pass a non-empty `files` array (or `targets` for framework-native names) with a concrete example, and both response fields (`message` and `error`) enumerate every scope the guard covers. Both guards' rejection semantics are unchanged. - **Foreground Stage B verdict settlement** (`src/hooks/delegation-gate.ts`): a `[TESTED] | task-N | SKIPPED | ...` structured verdict (tests were NOT run — prohibited scope, framework detection none, missing test file) no longer emits `stage_b_failed`. The task stays in its Stage B eligible state (`reviewer_run`/`pre_check_passed`), the reviewer's APPROVED gate proof is preserved, and the `stageBCompletion` entry is untouched, so the architect can re-dispatch the test gate instead of forcing a coder rework of correct code. A warn log records the skip for the orchestrator. Genuine `FAIL` verdicts and `REVIEWED` rejections keep the existing `stage_b_failed` → `rework_required` semantics. - **Background Stage B ingestion** (`src/background/stage-b-gates.ts`): `structuredStageBVerdict` now returns `'skip'` for TESTED SKIPPED; `ingestBackgroundStageBCompletion` consumes the record with the new `skipped: true` result flag and fires no transition and no proof clearing. `StageBIngestionResult` gains the optional `skipped` field. - **Background advisory** (`src/background/completion-observer.ts`): a skipped ingestion publishes `skipped (tests not run) — re-dispatch the test gate; reviewer proof preserved; task remains Stage B eligible` instead of the generic `ingestion failed`, so operators can distinguish a retryable skip from a hard failure. @@ -15,6 +15,6 @@ An agent that followed the tool's own advice (`scope:"all"`) could not succeed ## Tests -- `tests/unit/tools/test-runner-scope-advice.test.ts` — guard-2 message directs to files/targets and never recommends the blocked scope (all three guarded scopes); guard-1 block and the pinned `error` text are characterized as unchanged. +- `tests/unit/tools/test-runner-scope-advice.test.ts` — guard-2 response fields direct to files/targets, enumerate all three guarded scopes, and never recommend the blocked scope; guard-1 block characterized as unchanged. - `tests/unit/hooks/delegation-gate-stage-b-skipped.test.ts` — SKIPPED leaves state `reviewer_run` with durable reviewer proof and the reviewer completion entry intact; FAIL and REVIEWED REJECTED still go `rework_required` with proof cleared. - `tests/unit/background/stage-b-gates-skipped-verdict.test.ts` — SKIPPED ingest returns `skipped: true`, no state mutation, proof preserved; FAIL and unparseable output keep the fail-closed rejection.