From ff509e80e1cb7fd557463ec7ed70b089f9778cdf Mon Sep 17 00:00:00 2001 From: Dane Schneider <269480009+daneschneider-oai@users.noreply.github.com> Date: Mon, 31 Aug 2026 14:41:23 -0700 Subject: [PATCH 01/10] fix(deep-scan): publish aggregates without worker downgrades --- .../mcp-app/src/artifact-scan-draft.ts | 55 ++- .../src/deep-scan/artifact-validation.ts | 22 +- .../mcp-app/src/deep-scan/coordinator.ts | 65 +++- .../mcp-app/src/deep-scan/worker-runner.ts | 12 +- .../mcp-app/templates/deep-scan/dedup.md | 2 +- .../tests/deep_scan_publication_cases.mjs | 140 ++++++++ .../tests/test_artifact_deep_reducer.mjs | 81 ++++- .../tests/test_artifact_scan_draft.mjs | 179 ++++++++++ .../test_deep_scan_artifact_validation.mjs | 123 +++++-- .../tests/test_deep_scan_coordinator.mjs | 5 + .../codex-security/scripts/workbench_db.py | 6 +- .../test_deep_scan_successful_publication.py | 330 ++++++++++++++++++ 12 files changed, 940 insertions(+), 80 deletions(-) create mode 100644 plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs create mode 100644 plugins/codex-security/tests/test_deep_scan_successful_publication.py diff --git a/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts b/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts index 5b9ce3504..2e9e1469a 100644 --- a/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts +++ b/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts @@ -57,7 +57,7 @@ interface PreparedScanDraft { type PublishScanDraft = ( draft: PreparedScanDraft, - expectedDigest: string, + expectedDigest: string | undefined, checkpoint: ScanDraftInput, ) => Promise; @@ -88,7 +88,11 @@ export async function recordCodexSecurityScanDraft( for (;;) { signal?.throwIfAborted(); - const preserved = await preserveScanDraft(context, parsed, false); + // The coordinator has already accepted and reconciled a terminal Deep + // aggregate. Publishing it must not reopen checkpoints or prior drafts. + const preserved = context.mode === "deep" && parsed.complete !== false + ? { input: parsed, previousDigest: undefined } + : await preserveScanDraft(context, parsed, false); const reconciled = preserved.input; const contract = requireObject( context.targetContract, @@ -104,7 +108,7 @@ export async function recordCodexSecurityScanDraft( ); const target = buildTarget(context, contract, trustedTarget); const scope = buildScope(context, trustedScope, reconciled.scope); - const findings = buildFindings(reconciled.findings); + const findings = buildFindings(reconciled.findings, context.mode); const coverage = buildCoverage( context, contract, @@ -190,9 +194,10 @@ export async function recordCodexSecurityScanDraftViaWorkbench( draftPath, "--checkpoint-path", checkpointPath, - "--expected-draft-digest", - expectedDigest, ]; + if (expectedDigest !== undefined) { + arguments_.push("--expected-draft-digest", expectedDigest); + } if (context.handoffClaimToken) { arguments_.push("--claim-token", context.handoffClaimToken); } @@ -1388,7 +1393,7 @@ function buildScope( }; } -function buildFindings(findings: JsonObject[]): JsonObject[] { +function buildFindings(findings: JsonObject[], mode?: string): JsonObject[] { const generatedIdentities = findings.map((finding, index) => { if (finding.identity !== undefined) return undefined; const candidateId = (finding.extensions as JsonObject | undefined) @@ -1421,7 +1426,7 @@ function buildFindings(findings: JsonObject[]): JsonObject[] { ); } - return findings.map((finding, index) => { + const identified: JsonObject[] = findings.map((finding, index) => { const generatedIdentity = generatedIdentities[index]; if (generatedIdentity === undefined) return { ...finding }; const identity: JsonObject = { anchor: generatedIdentity.anchor }; @@ -1441,6 +1446,42 @@ function buildFindings(findings: JsonObject[]): JsonObject[] { identity, }; }); + if (mode !== "deep") return identified; + + // Independent workers can choose the same semantic identity for different + // findings. Assign distinct canonical instances without discarding evidence. + const reserved = new Set(identified.map(scanFindingIdentity)); + const used = new Set(); + return identified.map((finding) => { + const key = scanFindingIdentity(finding); + if (!used.has(key)) { + used.add(key); + return finding; + } + const identity = finding.identity as JsonObject; + const locations = (finding.locations as JsonObject[]).map((location) => JSON.stringify([ + location.path, location.startLine, location.endLine ?? location.startLine, + ])).sort(); + const digest = createHash("sha256") + .update(JSON.stringify([finding.ruleId, identity, locations])) + .digest("hex").slice(0, 16); + const baseInstance = `${identity.instance ?? "saved"}-${digest}`; + let suffix = 2; + const distinct: JsonObject & { identity: JsonObject } = { + ...finding, identity: { ...identity, instance: baseInstance }, + }; + while (reserved.has(scanFindingIdentity(distinct)) || used.has(scanFindingIdentity(distinct))) { + distinct.identity.instance = `${baseInstance}-${suffix}`; + suffix += 1; + } + const provenance = finding.provenance as JsonObject; + distinct.provenance = { + ...provenance, + preservedIdentity: provenance.preservedIdentity ?? structuredClone(identity), + }; + used.add(scanFindingIdentity(distinct)); + return distinct; + }); } function buildCoverage( diff --git a/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts b/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts index e26dd1cb3..8fabff02f 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts @@ -2,7 +2,6 @@ import { isDeepStrictEqual } from "node:util"; import { parsePersistedScanDraft, preserveFindingDetails, - preserveScanCoverage, saveScanDraftCheckpoint, scanFindingIdentity, type ScanDraftInput, @@ -17,6 +16,7 @@ export interface DeepReductionSources { export interface ReducerArtifactValidation { newFindings: number; + result: ScanDraftInput; } /** Admit exactly the complete semantic result written by an ordinary Standard scan. */ @@ -76,9 +76,11 @@ export async function validateReducerArtifacts(input: { await writeJsonAtomic(resultPath, result); } else { validateRetainedFindings(result, [], previous); + result.coverage = completedDeepScanCoverage(result.coverage); } const previousFindingIds = new Set((previous?.findings ?? []).map(scanFindingIdentity)); return { + result, newFindings: result.findings.filter((finding) => ( !previousFindingIds.has(scanFindingIdentity(finding)) )).length @@ -117,10 +119,9 @@ export function reconcileDeepReduction( } } retainSourceFindings(result, { discoveries, previous }); - result.coverage = preserveScanCoverage(result.coverage, [ - ...discoveries.map((discovery) => discovery.result.coverage), - ...(previous ? [previous.coverage] : []), - ]); + // Each worker is an independent review. Its unfinished review observations + // remain in its own saved result rather than becoming parent scan work. + result.coverage = completedDeepScanCoverage(result.coverage); if (result.threatModel === undefined) { const sourceModels = [ ...discoveries.map((discovery) => discovery.result.threatModel), @@ -154,6 +155,17 @@ export function reconcileDeepReduction( return result; } +function completedDeepScanCoverage(coverage: Record): Record { + return { + ...coverage, + completeness: "complete", + surfaces: (coverage.surfaces as Record[]).filter( + (surface) => surface.disposition !== "needs_follow_up", + ).map(({ receiptRefs: _workerReceipts, ...surface }) => surface), + deferred: [], + }; +} + function findingSourceIds(finding: Record): string[] { const provenance = finding.provenance as Record; const ids = provenance.sourceFindingIds; diff --git a/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts b/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts index 5e0d985b1..5b8aed015 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts @@ -3,13 +3,10 @@ import { promises as fs } from "node:fs"; import { basename, dirname, join } from "node:path"; import { createDeepScanArtifacts, - ensureDeepScanDirectories, - readJsonObject, - requireRegularFile + ensureDeepScanDirectories } from "./artifacts.js"; import { validateDiscoveryArtifacts, validateReducerArtifacts } from "./artifact-validation.js"; import { - parsePersistedScanDraft, scanDraftInputSchema, type ScanDraftInput } from "../artifact-scan-draft.js"; @@ -51,13 +48,16 @@ type SchedulerSettlement = | { status: "fulfilled"; outcome: SchedulerOutcome } | { status: "rejected"; error: unknown }; +type AcceptedReducer = Omit; + interface SchedulerResult { reason: DeepScanTerminalReason; omittedWorkerIds: string[]; canceledWorkerIds: string[]; accepted: AcceptedDiscovery[]; mergedWorkerIds: string[]; - reducers: SuccessfulDedupOutcome[]; + reducers: AcceptedReducer[]; + result?: ScanDraftInput; } type CoordinatorPhase = "setup" | "discovery" | "terminal"; @@ -68,7 +68,7 @@ interface SchedulerAudit { omittedWorkerIds: string[]; canceledWorkerIds: string[]; bufferedWorkerIds: string[]; - reducers: SuccessfulDedupOutcome[]; + reducers: AcceptedReducer[]; executions: WorkerExecutionAudit[]; } @@ -292,9 +292,8 @@ export class DeepScanCoordinator { const schedulerResult = await this.runScheduler(); if (this.canceled || this.externallyFailed) return; this.phase = "terminal"; - const latestReducer = schedulerResult.reducers.at(-1); - const draft = latestReducer - ? parsePersistedScanDraft(await readJsonObject(latestReducer.resultPath)) + const draft = schedulerResult.result + ? structuredClone(schedulerResult.result) : scanDraftInputSchema.parse({ scanId: this.state.scanId, findings: [], @@ -310,6 +309,26 @@ export class DeepScanCoordinator { if (draft.scanId !== this.state.scanId) { throw new Error("Deep Scan aggregate does not match its authoritative scan identity."); } + // Saturation can race with a worker's durable acceptance. Keep those + // already-validated findings without restarting discovery or reducing + // them again, and without reopening worker files during publication. + const omitted = new Set(schedulerResult.omittedWorkerIds); + for (const worker of schedulerResult.accepted) { + if (!omitted.has(worker.id)) continue; + for (const [index, finding] of worker.result.findings.entries()) { + const original = structuredClone(finding); + delete (original.provenance as Record).sourceFindingIds; + const sourceId = `${worker.id}:${index}`; + draft.findings.push({ + ...structuredClone(finding), + provenance: { + ...finding.provenance as Record, + sourceFindingIds: [sourceId], + sourceFindings: [{ id: sourceId, finding: original }], + }, + }); + } + } await this.options.onComplete?.(draft, this.publicationAbortController.signal); if (this.canceled || this.externallyFailed) return; this.state = await this.finishWithReplay(schedulerResult); @@ -605,7 +624,9 @@ export class DeepScanCoordinator { this.audit.mergedWorkerIds = mergedDiscoveries.map((worker) => worker.id); this.audit.canceledWorkerIds = [...canceledWorkerIds]; this.audit.executions = await this.recoverPersistedExecutions(); - const reducerOutcomes = await this.recoverCompletedReducers(recovered); + const recoveredReducers = await this.recoverCompletedReducers(recovered); + const reducerOutcomes = recoveredReducers.reducers; + let latestResult = recoveredReducers.result; let buffer: AcceptedDiscovery[] = recovered.filter((worker) => !mergedIds.has(worker.id)); let reducer: Promise | undefined; let previousReducerResultPath = reducerOutcomes.at(-1)?.resultPath; @@ -745,7 +766,9 @@ export class DeepScanCoordinator { this.state = outcome.run; previousReducerResultPath = outcome.resultPath; mergedDiscoveries.push(...outcome.consumed); - reducerOutcomes.push(outcome); + const { result: acceptedResult, ...metadata } = outcome; + latestResult = acceptedResult; + reducerOutcomes.push(metadata); this.audit.reducers = [...reducerOutcomes]; this.audit.mergedWorkerIds = unique(mergedDiscoveries.map((worker) => worker.id)); this.audit.bufferedWorkerIds = buffer.map((worker) => worker.id); @@ -907,7 +930,9 @@ export class DeepScanCoordinator { this.state = outcome.run; previousReducerResultPath = outcome.resultPath; mergedDiscoveries.push(...outcome.consumed); - reducerOutcomes.push(outcome); + const { result: acceptedResult, ...metadata } = outcome; + latestResult = acceptedResult; + reducerOutcomes.push(metadata); this.audit.reducers = [...reducerOutcomes]; this.audit.mergedWorkerIds = unique(mergedDiscoveries.map((worker) => worker.id)); this.audit.bufferedWorkerIds = buffer.map((worker) => worker.id); @@ -948,7 +973,8 @@ export class DeepScanCoordinator { canceledWorkerIds: unique(canceledWorkerIds), accepted, mergedWorkerIds: unique(mergedDiscoveries.map((worker) => worker.id)), - reducers: reducerOutcomes + reducers: reducerOutcomes, + result: latestResult, }; } @@ -959,7 +985,7 @@ export class DeepScanCoordinator { if (!worker.resultManifestPath || !worker.completionSequence) { throw new Error(`Accepted discovery ${worker.id} has incomplete persisted evidence.`); } - await validateDiscoveryArtifacts( + const result = await validateDiscoveryArtifacts( this.artifacts, worker.resultManifestPath, this.state.scanId @@ -970,6 +996,7 @@ export class DeepScanCoordinator { label: basename(dirname(worker.promptPath)), artifactDir: worker.artifactDir, resultPath: worker.resultManifestPath, + result, completionSequence: worker.completionSequence, attempt: worker.attempt, ...(worker.threadId ? { threadId: worker.threadId } : {}), @@ -981,10 +1008,11 @@ export class DeepScanCoordinator { private async recoverCompletedReducers( discoveries: AcceptedDiscovery[] - ): Promise { + ): Promise<{ reducers: AcceptedReducer[]; result?: ScanDraftInput }> { const discoveriesById = new Map(discoveries.map((worker) => [worker.id, worker])); const inputs = this.state.persistedDedupInputs ?? []; - const outcomes: SuccessfulDedupOutcome[] = []; + const outcomes: AcceptedReducer[] = []; + let latestResult: ScanDraftInput | undefined; const completedReducers = (this.state.persistedWorkers ?? []) .filter((worker) => worker.kind === "dedup" && worker.status === "succeeded") .sort((left, right) => ( @@ -1004,13 +1032,14 @@ export class DeepScanCoordinator { throw new Error(`Completed reducer ${worker.id} has incomplete persisted inputs.`); } const accepted = consumed as AcceptedDiscovery[]; - const { newFindings } = await validateReducerArtifacts({ + const { newFindings, result } = await validateReducerArtifacts({ artifacts: this.artifacts, artifactDir: worker.artifactDir, resultPath: worker.resultManifestPath, reducerId: worker.id, previousReducerResultPath: outcomes.at(-1)?.resultPath }, this.state.scanId); + latestResult = result; noNewStreak = newFindings > 0 ? 0 : noNewStreak + accepted.length; const evidence = await persistedWorkerEvidence(worker); outcomes.push({ @@ -1025,7 +1054,7 @@ export class DeepScanCoordinator { run: { ...this.state, noNewStreak } }); } - return outcomes; + return { reducers: outcomes, result: latestResult }; } private async recoverPersistedExecutions(): Promise { diff --git a/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts b/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts index 4c13886ca..bde3f7efc 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts @@ -2,6 +2,7 @@ import { createHash } from "node:crypto"; import { promises as fs } from "node:fs"; import { dirname, join } from "node:path"; import { getCodexSecurityDeepReducerInputs } from "../artifact-deep-reducer.js"; +import type { ScanDraftInput } from "../artifact-scan-draft.js"; import { validateDiscoveryArtifacts, validateReducerArtifacts @@ -38,6 +39,7 @@ export interface AcceptedDiscovery { label: string; artifactDir: string; resultPath: string; + result: ScanDraftInput; completionSequence: number; attempt: number; threadId?: string; @@ -62,6 +64,7 @@ export interface SuccessfulDedupOutcome { id: string; consumed: AcceptedDiscovery[]; resultPath: string; + result: ScanDraftInput; newFindings: number; attempt: number; threadId?: string; @@ -174,7 +177,7 @@ export class DeepScanWorkerRunner { artifactDir, attempt: 1 }); - let discoveryValidated = false; + let discoveryResult: ScanDraftInput | undefined; let outcome = await this.runWorkerWithRetries({ workerId, kind: "discovery", @@ -184,8 +187,7 @@ export class DeepScanWorkerRunner { artifactContext: { root: artifactDir, layout: "worker" }, subagents: run.config.subagents, validate: async () => { - await validateDiscoveryArtifacts(artifacts, files.resultPath, run.scanId); - discoveryValidated = true; + discoveryResult = await validateDiscoveryArtifacts(artifacts, files.resultPath, run.scanId); }, beforeRetry: async (attempt) => { await archiveDirectory( @@ -203,7 +205,7 @@ export class DeepScanWorkerRunner { }, outcome.attempt, outcome.threadId); outcome = { ...outcome, status: "canceled" }; } - if (!discoveryValidated) { + if (!discoveryResult) { await fs.rm(files.resultPath, { force: true }); } const basePromptSha256 = sha256(basePrompt); @@ -280,6 +282,7 @@ export class DeepScanWorkerRunner { label: workerLabel, artifactDir, resultPath: files.resultPath, + result: discoveryResult!, completionSequence: persisted.completionSequence, attempt: outcome.attempt, threadId: outcome.threadId, @@ -442,6 +445,7 @@ export class DeepScanWorkerRunner { id: reducerId, consumed, resultPath, + result: reducerValidation.result, newFindings: reducerValidation.newFindings, attempt: outcome.attempt, threadId: outcome.threadId, diff --git a/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md b/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md index b32585c22..443bdd566 100644 --- a/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md +++ b/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md @@ -16,6 +16,6 @@ For a valid merge, synthesize one stronger finding while preserving every materi Account for every input finding using the host-supplied `provenance.sourceFindingIds`. Copy the refs for retained findings; union them for a valid merge, preserving previous refs. Never invent, omit, or reuse a ref across independent output findings. The host retains original source payloads and rejects unaccounted input. Identity collisions do not establish that findings are duplicates. -Combine the complete coverage, exclusions, deferred work, open questions, threat-model context, and optional scope from the Standard results without dropping meaningful information. Keep coverage partial whenever any deferred work or follow-up surface requires it. You cannot resolve or reject a source finding without inspecting code, which is outside this reducer's role. +Summarize the reviewed surfaces, explicit exclusions, open questions, threat-model context, and optional scope in the aggregate. Individual workers are independent reviews: do not carry their partial or unknown coverage, deferred work, needs_follow_up surfaces, or receiptRefs into parent coverage. Their original results retain those observations and receipt references. Use complete coverage with an empty deferred array for the accepted aggregate; include only actually reviewed surface dispositions, never relabel unresolved work as reviewed. The host owns the Deep Scan outcome. You cannot resolve or reject a source finding without inspecting code, which is outside this reducer's role. Call `record_codex_security_deep_reduction({ scanId, findings, coverage, threatModel?, scope? })` with one complete Standard semantic result until it succeeds; correct a reported validation error and retry in the same conversation. After the first successful call, do not call it again. The host derives convergence and worker attribution from its existing state. diff --git a/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs b/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs new file mode 100644 index 000000000..79db21921 --- /dev/null +++ b/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs @@ -0,0 +1,140 @@ +import assert from "node:assert/strict"; +import { readFile, rm, writeFile } from "node:fs/promises"; +import path from "node:path"; + +export async function testDeepScanPublication({ + fixtureRun, FakeStore, FakeExecutor, DeepScanCoordinator, deferred, + immediateClock, eventually, standardScanDraft, +}) { + async function testSaturationPublishesFindingAcceptedDuringCancellation() { + const fixture = await fixtureRun({ workers: 3, subagents: 0, stopAfterNoNew: 2, maxDiscoveryRuns: 3 }); + const store = new FakeStore(fixture.run); + const releaseLateWorker = deferred(); + const lateAcceptance = deferred(); + const releaseAcceptance = deferred(); + const updateWorker = store.updateWorker.bind(store); + let acceptedLateWorker; + store.updateWorker = async (update) => { + const persisted = await updateWorker(update); + if (update.kind === "discovery" && update.status === "succeeded" + && path.basename(path.dirname(update.promptPath)) === "discovery-0003") { + acceptedLateWorker = persisted; + // Publication must use the result already validated before acceptance. + await rm(update.resultManifestPath); + lateAcceptance.resolve(); + await releaseAcceptance.promise; + } + return persisted; + }; + const executor = new FakeExecutor({ + blockDedup: true, + discoveryGates: { "discovery-0003": releaseLateWorker.promise }, + discoveryCandidates: { "discovery-0003": "late-accepted-finding" }, + }); + const completed = []; + const coordinator = new DeepScanCoordinator({ + run: fixture.run, store, executor, pluginRoot: fixture.pluginRoot, + clock: immediateClock, + onComplete: async (draft) => completed.push(structuredClone(draft)), + }); + coordinator.start(); + await executor.dedupStarted; + releaseLateWorker.resolve(); + await lateAcceptance.promise; + executor.releaseDedup(); + await eventually(() => executor.dedupSignal?.aborted === true); + releaseAcceptance.resolve(); + const terminal = await coordinator.wait(undefined, 5_000); + assert.equal(terminal?.status, "succeeded", terminal?.error); + assert.equal(terminal.terminalReason, "saturated"); + assert.equal(executor.discoveryCalls, 3); + assert.equal(executor.dedupCalls, 1, "late accepted results do not restart convergence"); + assert.deepEqual(store.finishCalls[0].omittedWorkerIds, [acceptedLateWorker.id]); + assert.equal(completed.length, 1); + assert.equal(completed[0].coverage.completeness, "complete"); + const [finding] = completed[0].findings; + assert.equal(completed[0].findings.length, 1); + assert.equal(finding.provenance.candidateId, "late-accepted-finding"); + assert.deepEqual(finding.provenance.sourceFindingIds, [`${acceptedLateWorker.id}:0`]); + assert.deepEqual(finding.provenance.sourceFindings, [{ + id: `${acceptedLateWorker.id}:0`, + finding: standardScanDraft(fixture.run.scanId, "late-accepted-finding", "discovery-0003").findings[0], + }]); + } + + async function testSuccessfulDeepCoverageIgnoresWorkerAndReducerReviewStatus() { + const fixture = await fixtureRun({ workers: 2, subagents: 0, stopAfterNoNew: 2, maxDiscoveryRuns: 2 }); + const store = new FakeStore(fixture.run); + const executor = new FakeExecutor(); + const run = executor.run.bind(executor); + const reviewed = { label: "Reviewed query", disposition: "no_issue_found" }; + const workerReviewed = { ...reviewed, receiptRefs: ["artifacts/missing-worker-receipt.md"] }; + const followUp = { label: "Worker follow-up", disposition: "needs_follow_up" }; + executor.run = async (request) => { + const outcome = await run(request); + const resultPath = path.join(request.artifactContext.root, "result.json"); + const draft = JSON.parse(await readFile(resultPath, "utf8")); + draft.coverage = { + completeness: request.kind === "discovery" && request.promptPath.includes("discovery-0002") + ? "unknown" : "partial", + surfaces: [workerReviewed, followUp], + explicitExclusions: [], + deferred: [{ reason: "An independent review left this question unresolved." }], + }; + await writeFile(resultPath, JSON.stringify(draft)); + return outcome; + }; + const completed = []; + const coordinator = new DeepScanCoordinator({ + run: fixture.run, store, executor, pluginRoot: fixture.pluginRoot, + clock: immediateClock, + onComplete: async (draft) => completed.push(structuredClone(draft)), + }); + coordinator.start(); + const terminal = await coordinator.wait(undefined, 5_000); + assert.equal(terminal?.status, "succeeded", terminal?.error); + assert.equal(completed.length, 1); + assert.deepEqual(completed[0].coverage, { + completeness: "complete", surfaces: [reviewed], explicitExclusions: [], deferred: [], + }); + for (const worker of store.workers.values()) { + if (worker.kind !== "discovery") continue; + const draft = JSON.parse(await readFile(worker.resultManifestPath, "utf8")); + assert.notEqual(draft.coverage.completeness, "complete"); + assert.deepEqual(draft.coverage.surfaces, [workerReviewed, followUp]); + assert.equal(draft.coverage.deferred.length, 1); + } + } + + async function testPublicationUsesAcceptedReducerSnapshot() { + const fixture = await fixtureRun({ workers: 1, subagents: 0, stopAfterNoNew: 1, maxDiscoveryRuns: 1 }); + const store = new FakeStore(fixture.run); + const commitDedup = store.commitDedup.bind(store); + store.commitDedup = async (commit) => { + const accepted = await commitDedup(commit); + await rm(commit.resultManifestPath); + return accepted; + }; + store.finish = async (input) => { + store.finishCalls.push(input); + Object.assign(store.run, { status: "succeeded", terminalReason: input.reason, manifestPath: input.manifestPath }); + return structuredClone(store.run); + }; + const completed = []; + const coordinator = new DeepScanCoordinator({ + run: fixture.run, store, + executor: new FakeExecutor({ discoveryCandidateId: "accepted-finding" }), + pluginRoot: fixture.pluginRoot, clock: immediateClock, + onComplete: async (draft) => completed.push(structuredClone(draft)), + }); + coordinator.start(); + const terminal = await coordinator.wait(undefined, 5_000); + assert.equal(terminal?.status, "succeeded", terminal?.error); + assert.equal(completed[0].findings[0].provenance.candidateId, "accepted-finding"); + assert.equal(completed[0].coverage.completeness, "complete"); + } + + await testSaturationPublishesFindingAcceptedDuringCancellation(); + await testSuccessfulDeepCoverageIgnoresWorkerAndReducerReviewStatus(); + await testPublicationUsesAcceptedReducerSnapshot(); +} diff --git a/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs b/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs index 2dd7c4cac..547e7b81e 100644 --- a/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs +++ b/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs @@ -86,6 +86,11 @@ try { }; const inputs = await getCodexSecurityDeepReducerInputs(context); + await assert.rejects( + recordCodexSecurityDeepReduction(context, draft([], { complete: false })), + /only a checkpoint/, + "host-owned coverage must not accept an unfinished reducer result", + ); await assert.rejects( recordCodexSecurityDeepReduction(context, draft([shared])), /unaccounted|discarded.*finding/, @@ -150,7 +155,8 @@ try { const rejectedCoverage = { completeness: "partial", surfaces: [ - { label: "SQL route", disposition: "rejected", notes: "Parameterized queries prevent injection." }, + { label: "SQL route", disposition: "rejected", notes: "Parameterized queries prevent injection.", + receiptRefs: ["artifacts/missing-worker-receipt.md"] }, { label: "Archive upload", disposition: "needs_follow_up", notes: "The guard still needs review." }, ], explicitExclusions: [{ pattern: "vendor", reason: "Outside the requested source scope." }], @@ -161,17 +167,63 @@ try { workersRoot, label: "discovery-coverage", id: "worker-coverage", result: draft([], { coverage: rejectedCoverage }), completionSequence: 4, }); + const unknownCoverageWorker = await createWorker({ + workersRoot, label: "discovery-unknown", id: "worker-unknown", + result: draft([], { coverage: { ...draft([]).coverage, completeness: "unknown" } }), + completionSequence: 5, + }); + const coverageWorkers = [coverageWorker, unknownCoverageWorker]; + const originalWorkerArtifacts = await Promise.all( + coverageWorkers.map((worker) => readFile(worker.resultPath, "utf8")), + ); const coverageRoot = path.join(dedupRoot, "dedup-coverage", "output"); await mkdir(coverageRoot, { recursive: true }); const coverageContext = { ...context, root: coverageRoot, - deepReducer: { scanRoot, claimedWorkers: [coverageWorker] }, + deepReducer: { scanRoot, claimedWorkers: coverageWorkers }, }; + assert.deepEqual( + (await getCodexSecurityDeepReducerInputs(coverageContext)).discoveries, + coverageWorkers.map((worker) => ({ workerId: worker.id, result: withSourceRefs(worker) })), + "the reducer can still inspect each worker's original coverage evidence", + ); await recordCodexSecurityDeepReduction(coverageContext, draft([])); assert.deepEqual( JSON.parse(await readFile(path.join(coverageRoot, "result.json"), "utf8")).coverage, - rejectedCoverage, - "reduction preserves rejection reasons and unfinished review even when the model omits them", + draft([]).coverage, + "accepted reductions do not inherit worker coverage even when every worker is partial or unknown", + ); + + const submittedCoverage = { + completeness: "partial", + surfaces: [ + { label: "Response rendering", disposition: "no_issue_found", notes: "All outputs use contextual encoding.", + receiptRefs: ["artifacts/missing-worker-receipt.md"] }, + { label: "Upload parsing", disposition: "rejected", notes: "Archive entries are not extracted." }, + { label: "Alternate handler", disposition: "needs_follow_up", notes: "A worker suggested another review." }, + ], + explicitExclusions: [{ pattern: "generated", reason: "Generated files are outside the requested scope." }], + deferred: [{ reason: "Repeat the alternate handler review.", paths: ["src/alternate.ts"] }], + openQuestions: ["Should future scans include generated handlers?"], + }; + await recordCodexSecurityDeepReduction(coverageContext, draft([], { coverage: submittedCoverage })); + assert.deepEqual( + JSON.parse(await readFile(path.join(coverageRoot, "result.json"), "utf8")).coverage, + { + ...submittedCoverage, + completeness: "complete", + surfaces: [ + { label: "Response rendering", disposition: "no_issue_found", notes: "All outputs use contextual encoding." }, + submittedCoverage.surfaces[1], + ], + deferred: [], + }, + "the host retains reviewed descriptions without linking missing optional worker receipts", + ); + assert.deepEqual( + await Promise.all(coverageWorkers.map((worker) => readFile(worker.resultPath, "utf8"))), + originalWorkerArtifacts, + "completing aggregate coverage must not rewrite raw worker evidence", ); const collision = { ...independent, ruleId: shared.ruleId, identity: shared.identity }; @@ -258,9 +310,21 @@ try { const enrichedPrevious = structuredClone(mergedWithSources); enrichedPrevious.findings[0].summary = "The earlier reduction established an additional reachable output route."; enrichedPrevious.findings[0].validation = { summary: "Both output routes bypass the same encoding control." }; - await writeFile(path.join(outputRoot, "result.json"), JSON.stringify(enrichedPrevious)); + enrichedPrevious.coverage = rejectedCoverage; + const previousArtifact = JSON.stringify(enrichedPrevious); + await writeFile(path.join(outputRoot, "result.json"), previousArtifact); await recordCodexSecurityDeepReduction(nextContext, merged); const preservedEnrichment = JSON.parse(await readFile(path.join(nextOutputRoot, "result.json"), "utf8")); + assert.deepEqual( + preservedEnrichment.coverage, + merged.coverage, + "a previous reducer's partial coverage is not inherited by a subsequent accepted reduction", + ); + assert.equal( + await readFile(path.join(outputRoot, "result.json"), "utf8"), + previousArtifact, + "the original previous reduction remains available without rewriting its coverage", + ); assert.equal( preservedEnrichment.findings[0].provenance.previousFindings[0].summary, enrichedPrevious.findings[0].summary, @@ -293,6 +357,13 @@ try { /repeats assigned Standard scan worker/ ); + await writeFile(first.resultPath, JSON.stringify({ ...first.result, complete: false })); + await assert.rejects( + getCodexSecurityDeepReducerInputs(context), + /only a checkpoint/, + "host-owned coverage must not admit unfinished Standard worker results", + ); + await writeFile(first.resultPath, "{invalid Standard scan\n"); await assert.rejects( getCodexSecurityDeepReducerInputs(context), diff --git a/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs b/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs index 77ff8a93d..429c3b6b7 100644 --- a/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs +++ b/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs @@ -500,6 +500,127 @@ try { assert.deepEqual(carriedParentManifest.scan.scope.summary, input.scope.summary); assert.deepEqual(carriedParentManifest.scan.threatModel, input.threatModel); + const deepParentRoot = path.join(root, "accepted-deep-parent"); + await mkdir(deepParentRoot); + const deepParentContext = { + ...context, + root: deepParentRoot, + mode: "deep", + scope: "src", + targetContract: { + ...context.targetContract, + scope: { + requiredIncludePaths: ["src", "lib"], + requiredExcludePaths: ["vendor"], + }, + }, + }; + const obsoleteFinding = { + ...finding, + identity: { anchor: "obsolete-parent-finding" }, + provenance: { ...finding.provenance, candidateId: "obsolete-parent-candidate" }, + extensions: { candidateId: "obsolete-parent-candidate" }, + }; + const obsoleteDeepDraft = { + ...input, + complete: false, + findings: [obsoleteFinding], + coverage: { + ...coverage, + completeness: "partial", + surfaces: [{ + label: "Old upload handler", + disposition: "needs_follow_up", + notes: "An earlier parent draft left this review unfinished.", + }], + deferred: [{ candidateId: "obsolete-review", reason: "Earlier review work." }], + }, + }; + await recordCodexSecurityScanDraft(deepParentContext, obsoleteDeepDraft); + await saveScanDraftCheckpoint(deepParentContext, { + ...obsoleteDeepDraft, + findings: [interruptedFinding], + }); + await recordCodexSecurityScanDraft(deepParentContext, { + ...input, + complete: false, + findings: [], + }); + assert.deepEqual( + new Set((await readJson(deepParentRoot, "findings.json")).findings.map( + (item) => item.provenance.candidateId, + )), + new Set(["obsolete-parent-candidate", "interrupted-checkpoint"]), + "an unfinished Deep parent checkpoint still preserves earlier validated findings", + ); + assert.equal((await readJson(deepParentRoot, "scan-manifest.json")).scan.complete, false); + assert.equal((await readJson(deepParentRoot, "coverage.json")).completeness, "partial"); + const savedDeepCheckpoints = await Promise.all( + (await readdir(path.join(deepParentRoot, "checkpoints"))).map(async (name) => [ + name, + await readFile(path.join(deepParentRoot, "checkpoints", name), "utf8"), + ]), + ); + const acceptedDeepDraft = { ...input, complete: true }; + await recordCodexSecurityScanDraft(deepParentContext, acceptedDeepDraft); + const acceptedDeepFindings = await readJson(deepParentRoot, "findings.json"); + const acceptedDeepCoverage = await readJson(deepParentRoot, "coverage.json"); + const acceptedDeepManifest = await readJson(deepParentRoot, "scan-manifest.json"); + assert.equal(acceptedDeepFindings.findings.length, 1); + assert.deepEqual(acceptedDeepFindings.findings[0].provenance, finding.provenance); + assert.equal(acceptedDeepCoverage.completeness, "complete"); + assert.deepEqual(acceptedDeepCoverage.deferred, []); + assert.deepEqual( + acceptedDeepCoverage.surfaces.map(({ label, disposition }) => ({ label, disposition })), + [{ label: "Archive extraction", disposition: "reported" }], + "accepted Deep coverage does not inherit obsolete parent review work", + ); + assert.deepEqual(acceptedDeepCoverage.includePaths, ["src", "lib"]); + assert.deepEqual(acceptedDeepCoverage.excludePaths, ["vendor"]); + assert.deepEqual(acceptedDeepManifest.scan.scope.includePaths, ["src", "lib"]); + assert.deepEqual(acceptedDeepManifest.scan.scope.excludePaths, ["vendor"]); + for (const [name, contents] of savedDeepCheckpoints) { + assert.equal(await readFile(path.join(deepParentRoot, "checkpoints", name), "utf8"), contents); + } + + const obsoleteCheckpointPath = path.join(deepParentRoot, "checkpoints", "obsolete.json"); + await writeFile(obsoleteCheckpointPath, "{malformed obsolete checkpoint\n"); + let deepPublicationCount = 0; + await recordCodexSecurityScanDraft( + deepParentContext, + acceptedDeepDraft, + async (draft, expectedDigest) => { + deepPublicationCount += 1; + assert.equal(expectedDigest, undefined); + assert.deepEqual(draft.findings, acceptedDeepFindings); + assert.deepEqual(draft.coverage, acceptedDeepCoverage); + }, + ); + assert.equal(deepPublicationCount, 1, "obsolete malformed checkpoints cannot block accepted Deep publication"); + + await writeFile(obsoleteCheckpointPath, ""); + let deepWorkbenchWrites = 0; + await recordCodexSecurityScanDraftViaWorkbench( + deepParentContext, + input, + async (arguments_) => { + deepWorkbenchWrites += 1; + assert.deepEqual(arguments_.slice(0, 3), ["write-scan-draft", "--scan-id", scanId]); + assert.equal(arguments_.includes("--expected-draft-digest"), false); + assert.deepEqual(arguments_.slice(-2), ["--claim-token", claimToken]); + const draftPath = arguments_[arguments_.indexOf("--draft-path") + 1]; + const checkpointPath = arguments_[arguments_.indexOf("--checkpoint-path") + 1]; + const staged = JSON.parse(await readFile(draftPath, "utf8")); + const stagedCheckpoint = JSON.parse(await readFile(checkpointPath, "utf8")); + assert.deepEqual(staged.findings, acceptedDeepFindings); + assert.deepEqual(staged.coverage, acceptedDeepCoverage); + assert.deepEqual(stagedCheckpoint.findings, input.findings); + assert.equal(stagedCheckpoint.handoffClaimToken, undefined); + }, + ); + assert.equal(deepWorkbenchWrites, 1, "terminal Deep drafts still publish through the workbench lock despite obsolete empty checkpoints"); + assert.deepEqual(await readdir(path.join(deepParentRoot, "drafts")), []); + const pendingRoot = path.join(root, "pending-worker"); await mkdir(pendingRoot); const pendingContext = { ...workerContext, root: pendingRoot }; @@ -2188,6 +2309,64 @@ try { "duplicate stable instance sources remain collisions for finalization", ); + const collisionFindings = ["src/upload.py", "src/import.py"].map((location) => ({ + ...finding, + locations: [{ path: location, startLine: 41, endLine: 44 }], + provenance: { source: "local_plugin" }, + extensions: {}, + })); + const authoredCollisionIdentity = { anchor: "shared-archive-review" }; + const collisionCases = [ + { + label: "authored", + findings: collisionFindings.map((item) => ({ ...item, identity: authoredCollisionIdentity })), + originalIdentity: authoredCollisionIdentity, + }, + { + label: "generated", + findings: collisionFindings, + originalIdentity: { anchor: "unsafe-archive-extraction", instance: "unsafe-archive-extraction" }, + }, + ]; + for (const collisionCase of collisionCases) { + const collisionInput = { ...input, findings: collisionCase.findings }; + await recordFreshScanDraft(context, collisionInput); + const standardFindings = (await readJson(root, "findings.json")).findings; + assert.deepEqual( + standardFindings.map((item) => item.identity), + [collisionCase.originalIdentity, collisionCase.originalIdentity], + `${collisionCase.label} identity collisions retain the existing Standard shape`, + ); + assert.deepEqual( + standardFindings.map((item) => item.provenance), + collisionCase.findings.map((item) => item.provenance), + ); + + const deepIdentityContext = { ...context, mode: "deep" }; + await recordFreshScanDraft(deepIdentityContext, collisionInput); + const deepFindings = (await readJson(root, "findings.json")).findings; + const deepIdentities = deepFindings.map((item) => item.identity); + assert.equal(deepFindings.length, 2, `${collisionCase.label} identity collisions must retain both Deep findings`); + assert.equal(new Set(deepIdentities.map((identity) => JSON.stringify(identity))).size, 2); + assert.deepEqual(deepFindings[0].identity, collisionCase.originalIdentity); + assert.deepEqual(deepFindings[0].provenance, collisionCase.findings[0].provenance); + assert.deepEqual(deepFindings[1].provenance, { + ...collisionCase.findings[1].provenance, + preservedIdentity: collisionCase.originalIdentity, + }); + assert.deepEqual( + deepFindings.map((item) => item.locations), + collisionCase.findings.map((item) => item.locations), + ); + + await recordCodexSecurityScanDraft(deepIdentityContext, collisionInput); + assert.deepEqual( + (await readJson(root, "findings.json")).findings.map((item) => item.identity), + deepIdentities, + `${collisionCase.label} Deep finding identities remain stable when the accepted aggregate is republished`, + ); + } + const completeCodeEvidence = { ...incompleteCodeEvidence, code: "extract_archive_entry(untrusted_entry, output_path)", diff --git a/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs b/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs index e5c3e6b65..2144f4837 100644 --- a/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs +++ b/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs @@ -210,13 +210,19 @@ async function testReducerValidation(root) { await assert.rejects(validateSnapshot(), /unaccounted source findings/); await writeResult(resultPath, draft([firstFinding, secondFinding])); await writeFile(first.resultPath, "{source changed after dispatch"); - assert.deepEqual(await validateSnapshot(), { newFindings: 2 }); + const validatedSnapshot = await validateSnapshot(); + assert.equal(validatedSnapshot.newFindings, 2); const admitted = JSON.parse(await readFile(resultPath, "utf8")); + assert.deepEqual( + validatedSnapshot.result, + admitted, + "validation returns the same reconciled result that was accepted on disk", + ); assert.deepEqual(admitted.findings[1].provenance.sourceFindingIds, ["worker-001:1"]); sources.previous = structuredClone(admitted); sources.previous.findings[0].summary = "Additional proof established by the previous reducer."; await writeResult(resultPath, draft([firstFinding, secondFinding])); - assert.deepEqual(await validateSnapshot(), { newFindings: 0 }); + assert.equal((await validateSnapshot()).newFindings, 0); assert.equal( JSON.parse(await readFile(resultPath, "utf8")).findings[0].provenance.previousFindings[0].summary, sources.previous.findings[0].summary, @@ -294,7 +300,7 @@ async function testReducerValidation(root) { deferred: [], }, })); - await validateReducerArtifacts({ + const validatedCoverage = await validateReducerArtifacts({ artifacts, artifactDir, resultPath, @@ -305,14 +311,17 @@ async function testReducerValidation(root) { result: draft([firstFinding], { coverage: { completeness: "partial", - surfaces: [{ - riskArea: "filesystem", - notes: "The discovery worker still needed runtime validation.", - disposition: "needs_follow_up", - label: "Archive extraction", - }], + surfaces: [ + { + riskArea: "filesystem", + notes: "The discovery worker still needed runtime validation.", + disposition: "needs_follow_up", + label: "Archive extraction", + }, + { label: "Worker-only handler", disposition: "needs_follow_up" }, + ], explicitExclusions: [], - deferred: [], + deferred: [{ reason: "An independent worker suggested another review." }], }, }), }], @@ -321,11 +330,16 @@ async function testReducerValidation(root) { }, scanId); const reconciledCoverage = JSON.parse(await readFile(resultPath, "utf8")).coverage; assert.deepEqual( - reconciledCoverage.surfaces, - [resolvedCoverageSurface], - "the reducer's updated semantic surface must supersede stale source coverage", + reconciledCoverage, + { + completeness: "complete", + surfaces: [resolvedCoverageSurface], + explicitExclusions: [], + deferred: [], + }, + "the accepted aggregate retains its own coverage without inheriting worker review work", ); - assert.equal(reconciledCoverage.completeness, "complete"); + assert.deepEqual(validatedCoverage.result.coverage, reconciledCoverage); await writeResult(resultPath, draft([])); await assert.rejects( @@ -411,16 +425,49 @@ async function testReducerValidation(root) { ...(previousReducerResultPath ? { previousReducerResultPath } : {}) }, scanId); - assert.deepEqual(await validate(), { newFindings: 1 }); + assert.equal((await validate()).newFindings, 1); + + const legacyPartial = draft([firstFinding], { + coverage: { + completeness: "partial", + surfaces: [ + { ...resolvedCoverageSurface, receiptRefs: ["artifacts/missing-worker-receipt.md"] }, + { label: "Legacy follow-up", disposition: "needs_follow_up" }, + ], + explicitExclusions: [{ pattern: "vendor", reason: "Outside the requested source scope." }], + deferred: [{ reason: "A previous reducer retained worker follow-up work." }], + openQuestions: ["Should a future review include generated handlers?"], + }, + }); + await writeResult(resultPath, legacyPartial); + const legacyArtifact = await readFile(resultPath, "utf8"); + const resumed = await validate(); + assert.equal(resumed.newFindings, 1); + assert.deepEqual(resumed.result, { + ...legacyPartial, + coverage: { + ...legacyPartial.coverage, + completeness: "complete", + surfaces: [resolvedCoverageSurface], + deferred: [], + }, + }); + assert.equal( + await readFile(resultPath, "utf8"), + legacyArtifact, + "resuming a legacy partial reducer returns normalized coverage without rewriting its original artifact", + ); + await writeResult(resultPath, { ...legacyPartial, complete: false }); + await assert.rejects(validate(), /only a checkpoint|not complete/); await writeResult(resultPath, draft([])); - assert.deepEqual(await validate(), { newFindings: 0 }); + assert.equal((await validate()).newFindings, 0); await writeResult(resultPath, { ...draft([firstFinding]), resultPath: "/tmp/result.json" }); await assert.rejects(validate(), /invalid Standard scan result/); await writeResult(resultPath, draft([firstFinding, secondFinding])); - assert.deepEqual(await validate(), { newFindings: 2 }); + assert.equal((await validate()).newFindings, 2); const previousReducerResultPath = path.join( artifacts.dedupRoot, @@ -430,16 +477,16 @@ async function testReducerValidation(root) { ); await mkdir(path.dirname(previousReducerResultPath), { recursive: true }); await writeResult(previousReducerResultPath, draft([firstFinding])); - assert.deepEqual( - await validate(previousReducerResultPath), - { newFindings: 1 } + assert.equal( + (await validate(previousReducerResultPath)).newFindings, + 1 ); const renamedTitle = { ...firstFinding, title: "Stronger explanation of the same finding." }; await writeResult(resultPath, draft([renamedTitle, secondFinding])); - assert.deepEqual( - await validate(previousReducerResultPath), - { newFindings: 1 } + assert.equal( + (await validate(previousReducerResultPath)).newFindings, + 1 ); await writeResult(previousReducerResultPath, draft([firstFinding, secondFinding])); @@ -451,9 +498,9 @@ async function testReducerValidation(root) { const replacement = finding("replacement", "src/c.js"); await writeResult(resultPath, draft([firstFinding, secondFinding, replacement])); - assert.deepEqual( - await validate(previousReducerResultPath), - { newFindings: 1 } + assert.equal( + (await validate(previousReducerResultPath)).newFindings, + 1 ); const implicitFirst = { ...firstFinding }; @@ -461,9 +508,9 @@ async function testReducerValidation(root) { const implicitRenamed = { ...implicitFirst, summary: "More complete evidence." }; await writeResult(previousReducerResultPath, draft([implicitFirst])); await writeResult(resultPath, draft([implicitRenamed])); - assert.deepEqual( - await validate(previousReducerResultPath), - { newFindings: 0 } + assert.equal( + (await validate(previousReducerResultPath)).newFindings, + 0 ); await writeResult(resultPath, draft([{ ...implicitRenamed, @@ -472,9 +519,9 @@ async function testReducerValidation(root) { { path: "src/another-affected-location.js", startLine: 4 } ] }])); - assert.deepEqual( - await validate(previousReducerResultPath), - { newFindings: 0 }, + assert.equal( + (await validate(previousReducerResultPath)).newFindings, + 0, "An existing finding without an explicit identity may gain affected locations." ); @@ -487,15 +534,15 @@ async function testReducerValidation(root) { await writeResult(resultPath, draft([firstFinding])); await writeResult(first.resultPath, { ...draft([firstFinding]), scanId: otherScanId }); - assert.deepEqual( - await validate(), - { newFindings: 1 }, + assert.equal( + (await validate()).newFindings, + 1, "A completed aggregate must not reread already-consumed Standard results." ); await writeFile(first.resultPath, "{invalid Standard scan\n"); - assert.deepEqual( - await validate(), - { newFindings: 1 }, + assert.equal( + (await validate()).newFindings, + 1, "Accepted Standard inputs were already validated by the reducer writer." ); await writeResult(first.resultPath, draft([firstFinding])); diff --git a/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs b/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs index a27a03dde..4c8c383a9 100644 --- a/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs +++ b/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs @@ -4,6 +4,7 @@ import { mkdir, mkdtemp, readFile, readdir, realpath, rename, rm, writeFile } fr import { tmpdir } from "node:os"; import path from "node:path"; import { build } from "esbuild"; +import { testDeepScanPublication } from "./deep_scan_publication_cases.mjs"; const bundle = await build({ bundle: true, @@ -3962,6 +3963,10 @@ try { await testSandboxDiagnosticSurvivesArtifactRetries(); await testCompletionOrdering(); await testSaturationPreservesFindingAlreadyBuffered(); + await testDeepScanPublication({ + fixtureRun, FakeStore, FakeExecutor, DeepScanCoordinator, deferred, + immediateClock, eventually, standardScanDraft, + }); await testDirectReducerCannotDropAcceptedFinding(); await testSaturationDrainsBufferedAndCancelsInflight(); await testDiscoveryDeadlineDrainsActiveReducerAndPreservesFindings(); diff --git a/plugins/codex-security/scripts/workbench_db.py b/plugins/codex-security/scripts/workbench_db.py index b5227826f..7e04b1dda 100644 --- a/plugins/codex-security/scripts/workbench_db.py +++ b/plugins/codex-security/scripts/workbench_db.py @@ -1534,7 +1534,9 @@ def add_warning() -> None: scan_dir, expected_coverage_mode=expected_coverage_mode(scan), completion_binding=completion_binding, - completion_warnings=warnings, + # Deep has already accepted its aggregate. Publication must not + # recover worker-local drafts or revise the aggregate's coverage. + completion_warnings=warnings if scan["mode"] != "deep" else None, draft_documents=saved_results.merge_saved_results( scan_dir, scan["id"], @@ -1547,7 +1549,7 @@ def add_warning() -> None: stopped=False, reason="", ) - if current_manifest_path is not None and not already_sealed + if scan["mode"] != "deep" and current_manifest_path is not None and not already_sealed else None, ) add_warning() diff --git a/plugins/codex-security/tests/test_deep_scan_successful_publication.py b/plugins/codex-security/tests/test_deep_scan_successful_publication.py new file mode 100644 index 000000000..da924d6b8 --- /dev/null +++ b/plugins/codex-security/tests/test_deep_scan_successful_publication.py @@ -0,0 +1,330 @@ +from __future__ import annotations + +import copy +import json +import uuid +from argparse import Namespace +from pathlib import Path +from types import SimpleNamespace + +import pytest +from workbench_test_support import write_checkpoint, write_completed_contract + + +@pytest.fixture +def publication_scan(workbench_api, workbench_db, tmp_path, monkeypatch): + monkeypatch.setenv("CODEX_SECURITY_STATE_DIR", str(tmp_path / "state")) + monkeypatch.setenv("CODEX_HOME", str(tmp_path / "codex-home")) + deep = workbench_api["deep_scan"] + monkeypatch.setattr( + deep, + "_dependencies", + deep.DeepScanDependencies( + **{ + name: workbench_api[ + "preserve_stopped_results_after_transition" + if name == "preserve_stopped_results" + else name + ] + for name in deep.DeepScanDependencies.__dataclass_fields__ + } + ), + ) + + def create(*, mode="deep", scope="."): + target = tmp_path / "target" + target.mkdir() + (target / "subdir").mkdir() + (target / "subdir" / "extract.py").write_text("# Synthetic scan target\n") + scan_dir = tmp_path / "scan" + scan_dir.mkdir(mode=0o700) + registered = workbench_api["register_cli_scan"]( + workbench_db, + Namespace( + repository=str(target), + scan_dir=str(scan_dir), + recipe_json=json.dumps( + { + "config": {}, + "mode": mode, + "repository": str(target), + "target": { + "kind": "repository" if scope == "." else "paths", + "paths": [] if scope == "." else [scope], + }, + } + ), + registration_json_stdin=False, + recipe_json_stdin=False, + parent_scan_id=None, + archive_existing=False, + archived_scan_dir=None, + ), + ) + scan_id = registered["scanId"] + timestamp = workbench_db.execute( + "SELECT started_at FROM scans WHERE id = ?", (scan_id,) + ).fetchone()[0] + if mode == "deep": + # The coordinator has already accepted and submitted its final aggregate. + # Exercise publication independently of the worker execution lifecycle. + with workbench_db: + workbench_db.execute( + "INSERT INTO deep_scan_runs (scan_id, schema_version, workflow_version, " + "status, phase, workers, subagents, stop_after_no_new, max_discovery_runs, " + "manifest_path, terminal_reason, created_at, updated_at, completed_at) " + "VALUES (?, 1, 'publication-test', 'succeeded', 'terminal', 1, 0, 1, 1, " + "?, 'saturated', ?, ?, ?)", + ( + scan_id, + str(scan_dir / "scan-manifest.json"), + timestamp, + timestamp, + timestamp, + ), + ) + coverage_mode = ( + "scoped_path" if scope != "." else "deep_repository" if mode == "deep" else "repository" + ) + write_completed_contract( + scan_dir, + scan_id, + target, + include_paths=[scope], + relative_path="subdir/extract.py", + coverage_mode=coverage_mode, + inventory_strategy="scoped_path" if scope != "." else "repository", + ) + manifest = json.loads((scan_dir / "scan-manifest.json").read_text()) + findings = json.loads((scan_dir / "findings.json").read_text())["findings"] + coverage = json.loads((scan_dir / "coverage.json").read_text()) + coverage["openQuestions"] = [{"question": "Which deployment controls apply?"}] + for field in ("documentType", "schemaVersion", "scanId"): + coverage.pop(field) + # Match the ordinary semantic envelopes written by the host draft writer. + manifest = {"scan": {key: manifest["scan"][key] for key in ("target", "scope")}} + findings[0]["severity"]["changeConditions"] = "Reassess if the upload route is removed." + findings[0]["provenance"]["sourceFindings"] = [ + {"id": "review-1:candidate-1", "finding": {"summary": "Retained original wording."}} + ] + for name, value in ( + ("scan-manifest.json", manifest), + ("findings.json", {"findings": findings}), + ("coverage.json", coverage), + ): + (scan_dir / name).write_text(json.dumps(value)) + return SimpleNamespace( + scan_id=scan_id, + scan_dir=scan_dir, + timestamp=timestamp, + findings=findings, + coverage=coverage, + ) + + return create + + +def add_worker(connection, scan, *, status="succeeded") -> Path: + worker_id = str(uuid.uuid4()) + output = scan.scan_dir / "workers" / worker_id + output.mkdir(parents=True) + result = output / "result.json" + with connection: + connection.execute( + "INSERT INTO deep_scan_workers (id, scan_id, kind, status, merge_state, " + "prompt_path, artifact_dir, result_manifest_path, attempt, created_at, " + "updated_at, completed_at) VALUES (?, ?, 'discovery', ?, ?, ?, ?, ?, 1, ?, ?, ?)", + ( + worker_id, + scan.scan_id, + status, + "merged" if status == "succeeded" else "none", + str(output / "prompt.md"), + str(output), + str(result), + scan.timestamp, + scan.timestamp, + scan.timestamp, + ), + ) + return result + + +def complete(workbench_api, connection, scan, *, prepare_only=False): + return workbench_api["complete_scan"]( + connection, + Namespace(scan_id=scan.scan_id, claim_token=None, cost_json=None), + prepare_only=prepare_only, + )["scan"] + + +def assert_published_aggregate(scan): + findings = json.loads((scan.scan_dir / "findings.json").read_text())["findings"] + for finding in findings: + for field in ("findingId", "occurrenceId", "fingerprints"): + assert finding.pop(field) + assert findings == scan.findings + coverage = json.loads((scan.scan_dir / "coverage.json").read_text()) + for field in ("documentType", "schemaVersion", "scanId"): + coverage.pop(field) + assert coverage == scan.coverage + assert (scan.scan_dir / "report.md").is_file() + + +@pytest.mark.parametrize("scope", [".", "subdir"], ids=["repository", "scoped"]) +def test_deep_publication_uses_aggregate_without_worker_coverage( + workbench_api, workbench_db, publication_scan, scope +): + scan = publication_scan(scope=scope) + result = add_worker(workbench_db, scan) + worker_coverage = copy.deepcopy(scan.coverage) + worker_coverage["completeness"] = "partial" + worker_coverage["surfaces"][0]["disposition"] = "needs_follow_up" + worker_coverage["deferred"] = [{"id": "worker-follow-up", "reason": "Review this path again."}] + result.write_text( + json.dumps( + { + "scanId": scan.scan_id, + "complete": True, + "findings": [], + "coverage": worker_coverage, + } + ) + ) + source_bytes = result.read_bytes() + + completed = complete(workbench_api, workbench_db, scan) + + assert completed["progress"]["status"] == "complete" + assert_published_aggregate(scan) + assert result.read_bytes() == source_bytes + + +def test_deep_publication_ignores_empty_canceled_checkpoint( + workbench_api, workbench_db, publication_scan +): + scan = publication_scan() + result = add_worker(workbench_db, scan, status="canceled") + checkpoint = write_checkpoint( + result.parent / "checkpoints", + { + "scanId": scan.scan_id, + "complete": False, + "findings": [], + "coverage": { + "completeness": "partial", + "surfaces": [], + "explicitExclusions": [], + "deferred": [], + }, + }, + ) + source_bytes = checkpoint.read_bytes() + + complete(workbench_api, workbench_db, scan) + + assert_published_aggregate(scan) + assert checkpoint.read_bytes() == source_bytes + assert not result.exists() + + +@pytest.mark.parametrize("old_result", ["unreadable", "removed"]) +def test_deep_publication_does_not_require_old_worker_files( + workbench_api, workbench_db, publication_scan, old_result +): + scan = publication_scan() + result = add_worker(workbench_db, scan) + result.write_text("{old worker output is unavailable") + if old_result == "removed": + result.unlink() + + completed = complete(workbench_api, workbench_db, scan) + + assert completed["warnings"] == [] + assert_published_aggregate(scan) + if old_result == "removed": + assert not result.exists() + else: + assert result.read_text() == "{old worker output is unavailable" + + +def test_deep_prepare_and_complete_preserve_the_same_aggregate( + workbench_api, workbench_db, publication_scan +): + scan = publication_scan() + prepared = complete(workbench_api, workbench_db, scan, prepare_only=True) + assert prepared["progress"]["status"] == "running" + names = ("scan-manifest.json", "findings.json", "coverage.json") + published = {name: (scan.scan_dir / name).read_bytes() for name in names} + + complete(workbench_api, workbench_db, scan, prepare_only=True) + complete(workbench_api, workbench_db, scan) + repeated = complete(workbench_api, workbench_db, scan) + + assert repeated["progress"]["status"] == "complete" + assert {name: (scan.scan_dir / name).read_bytes() for name in names} == published + assert_published_aggregate(scan) + + +def test_stopped_deep_scan_still_salvages_checkpoint_findings( + workbench_api, workbench_db, publication_scan +): + scan = publication_scan() + with workbench_db: + workbench_db.execute( + "UPDATE deep_scan_runs SET status = 'running', phase = 'reducing', " + "terminal_reason = NULL, completed_at = NULL WHERE scan_id = ?", + (scan.scan_id,), + ) + result = add_worker(workbench_db, scan, status="running") + later_finding = copy.deepcopy(scan.findings[0]) + later_finding["identity"]["anchor"] = "later-checkpoint-finding" + later_finding["summary"] = "Finding saved after the last completed aggregate." + checkpoint = write_checkpoint( + result.parent / "checkpoints", + { + "scanId": scan.scan_id, + "complete": False, + "findings": [later_finding], + "coverage": { + "completeness": "partial", + "surfaces": [], + "explicitExclusions": [], + "deferred": [], + }, + }, + ) + result.write_text("{interrupted worker output") + + stopped = workbench_api["fail_scan"]( + workbench_db, + Namespace( + scan_id=scan.scan_id, claim_token=None, cost_json=None, message="Scan interrupted." + ), + )["scan"] + + assert stopped["progress"]["status"] == "failed" + manifest = json.loads((scan.scan_dir / "scan-manifest.json").read_text()) + coverage = json.loads((scan.scan_dir / "coverage.json").read_text()) + assert manifest["scan"]["status"] == "failed" + assert coverage["completeness"] == "partial" + findings = json.loads((scan.scan_dir / "findings.json").read_text())["findings"] + assert {finding["summary"] for finding in findings} == { + scan.findings[0]["summary"], + later_finding["summary"], + } + assert checkpoint.is_file() + assert result.read_text() == "{interrupted worker output" + + +def test_standard_publication_preserves_deliberately_partial_coverage( + workbench_api, workbench_db, publication_scan +): + scan = publication_scan(mode="standard") + scan.coverage["completeness"] = "partial" + scan.coverage["deferred"] = [{"id": "remaining-review", "reason": "Another surface remains."}] + (scan.scan_dir / "coverage.json").write_text(json.dumps(scan.coverage)) + + complete(workbench_api, workbench_db, scan) + + assert_published_aggregate(scan) From 6f74b4ec4a764eef0368e99bdc4d2d75887171f0 Mon Sep 17 00:00:00 2001 From: Dane Schneider <269480009+daneschneider-oai@users.noreply.github.com> Date: Mon, 31 Aug 2026 15:14:40 -0700 Subject: [PATCH 02/10] fix(deep-scan): discard unfinished workers at saturation --- .../mcp-app/src/deep-scan/coordinator.ts | 27 +--- .../mcp-app/src/deep-scan/worker-runner.ts | 9 +- .../tests/deep_scan_publication_cases.mjs | 66 ++++++-- .../tests/test_deep_scan_coordinator.mjs | 23 +-- .../scripts/deep_scan_workbench.py | 7 +- .../tests/test_deep_scan_stop_conditions.py | 150 ++++++++++++++++++ .../tests/test_workbench_deep_scan.py | 14 -- 7 files changed, 231 insertions(+), 65 deletions(-) create mode 100644 plugins/codex-security/tests/test_deep_scan_stop_conditions.py diff --git a/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts b/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts index 5b8aed015..89b7050f5 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts @@ -309,26 +309,6 @@ export class DeepScanCoordinator { if (draft.scanId !== this.state.scanId) { throw new Error("Deep Scan aggregate does not match its authoritative scan identity."); } - // Saturation can race with a worker's durable acceptance. Keep those - // already-validated findings without restarting discovery or reducing - // them again, and without reopening worker files during publication. - const omitted = new Set(schedulerResult.omittedWorkerIds); - for (const worker of schedulerResult.accepted) { - if (!omitted.has(worker.id)) continue; - for (const [index, finding] of worker.result.findings.entries()) { - const original = structuredClone(finding); - delete (original.provenance as Record).sourceFindingIds; - const sourceId = `${worker.id}:${index}`; - draft.findings.push({ - ...structuredClone(finding), - provenance: { - ...finding.provenance as Record, - sourceFindingIds: [sourceId], - sourceFindings: [{ id: sourceId, finding: original }], - }, - }); - } - } await this.options.onComplete?.(draft, this.publicationAbortController.signal); if (this.canceled || this.externallyFailed) return; this.state = await this.finishWithReplay(schedulerResult); @@ -959,7 +939,9 @@ export class DeepScanCoordinator { this.audit.canceledWorkerIds = unique(canceledWorkerIds); this.audit.bufferedWorkerIds = buffer.map((worker) => worker.id); - if (lateFailure) throw lateFailure; + // Saturation fixes the aggregate at the stop boundary. Failures from workers + // still settling after cancellation cannot overturn that completed result. + if (lateFailure && stopReason !== "saturated") throw lateFailure; if ( !previousReducerResultPath @@ -985,7 +967,7 @@ export class DeepScanCoordinator { if (!worker.resultManifestPath || !worker.completionSequence) { throw new Error(`Accepted discovery ${worker.id} has incomplete persisted evidence.`); } - const result = await validateDiscoveryArtifacts( + await validateDiscoveryArtifacts( this.artifacts, worker.resultManifestPath, this.state.scanId @@ -996,7 +978,6 @@ export class DeepScanCoordinator { label: basename(dirname(worker.promptPath)), artifactDir: worker.artifactDir, resultPath: worker.resultManifestPath, - result, completionSequence: worker.completionSequence, attempt: worker.attempt, ...(worker.threadId ? { threadId: worker.threadId } : {}), diff --git a/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts b/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts index bde3f7efc..c9c32bf67 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts @@ -39,7 +39,6 @@ export interface AcceptedDiscovery { label: string; artifactDir: string; resultPath: string; - result: ScanDraftInput; completionSequence: number; attempt: number; threadId?: string; @@ -177,7 +176,7 @@ export class DeepScanWorkerRunner { artifactDir, attempt: 1 }); - let discoveryResult: ScanDraftInput | undefined; + let discoveryValidated = false; let outcome = await this.runWorkerWithRetries({ workerId, kind: "discovery", @@ -187,7 +186,8 @@ export class DeepScanWorkerRunner { artifactContext: { root: artifactDir, layout: "worker" }, subagents: run.config.subagents, validate: async () => { - discoveryResult = await validateDiscoveryArtifacts(artifacts, files.resultPath, run.scanId); + await validateDiscoveryArtifacts(artifacts, files.resultPath, run.scanId); + discoveryValidated = true; }, beforeRetry: async (attempt) => { await archiveDirectory( @@ -205,7 +205,7 @@ export class DeepScanWorkerRunner { }, outcome.attempt, outcome.threadId); outcome = { ...outcome, status: "canceled" }; } - if (!discoveryResult) { + if (!discoveryValidated) { await fs.rm(files.resultPath, { force: true }); } const basePromptSha256 = sha256(basePrompt); @@ -282,7 +282,6 @@ export class DeepScanWorkerRunner { label: workerLabel, artifactDir, resultPath: files.resultPath, - result: discoveryResult!, completionSequence: persisted.completionSequence, attempt: outcome.attempt, threadId: outcome.threadId, diff --git a/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs b/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs index 79db21921..8ca603be1 100644 --- a/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs +++ b/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs @@ -4,9 +4,9 @@ import path from "node:path"; export async function testDeepScanPublication({ fixtureRun, FakeStore, FakeExecutor, DeepScanCoordinator, deferred, - immediateClock, eventually, standardScanDraft, + immediateClock, eventually, }) { - async function testSaturationPublishesFindingAcceptedDuringCancellation() { + async function testSaturationOmitsWorkerAcceptedDuringCancellation() { const fixture = await fixtureRun({ workers: 3, subagents: 0, stopAfterNoNew: 2, maxDiscoveryRuns: 3 }); const store = new FakeStore(fixture.run); const releaseLateWorker = deferred(); @@ -19,7 +19,7 @@ export async function testDeepScanPublication({ if (update.kind === "discovery" && update.status === "succeeded" && path.basename(path.dirname(update.promptPath)) === "discovery-0003") { acceptedLateWorker = persisted; - // Publication must use the result already validated before acceptance. + // A worker still settling when saturation is reached is omitted. await rm(update.resultManifestPath); lateAcceptance.resolve(); await releaseAcceptance.promise; @@ -52,14 +52,7 @@ export async function testDeepScanPublication({ assert.deepEqual(store.finishCalls[0].omittedWorkerIds, [acceptedLateWorker.id]); assert.equal(completed.length, 1); assert.equal(completed[0].coverage.completeness, "complete"); - const [finding] = completed[0].findings; - assert.equal(completed[0].findings.length, 1); - assert.equal(finding.provenance.candidateId, "late-accepted-finding"); - assert.deepEqual(finding.provenance.sourceFindingIds, [`${acceptedLateWorker.id}:0`]); - assert.deepEqual(finding.provenance.sourceFindings, [{ - id: `${acceptedLateWorker.id}:0`, - finding: standardScanDraft(fixture.run.scanId, "late-accepted-finding", "discovery-0003").findings[0], - }]); + assert.deepEqual(completed[0].findings, [], "late worker findings are not appended to the saturated aggregate"); } async function testSuccessfulDeepCoverageIgnoresWorkerAndReducerReviewStatus() { @@ -106,6 +99,54 @@ export async function testDeepScanPublication({ } } + async function testSaturationIgnoresDiscoveryCancellationWriteFailure() { + const fixture = await fixtureRun({ workers: 2, subagents: 0, stopAfterNoNew: 2, maxDiscoveryRuns: 6 }); + const store = new FakeStore(fixture.run); + const executor = new FakeExecutor({ blockDedup: true, blockDiscoveryAfterCalls: 2 }); + const updateWorker = store.updateWorker.bind(store); + const rejectedCancellations = new Set(); + store.updateWorker = async (update) => { + if (update.kind === "discovery" && update.status === "canceled") { + assert.equal(executor.dedupSignal?.aborted, true); + assert.equal(store.run.noNewStreak, 2); + rejectedCancellations.add(update.id); + throw new Error("fixture cancellation persistence failure"); + } + return updateWorker(update); + }; + const completed = []; + const coordinator = new DeepScanCoordinator({ + run: fixture.run, store, executor, pluginRoot: fixture.pluginRoot, + clock: immediateClock, + onComplete: async (draft) => completed.push(structuredClone(draft)), + }); + coordinator.start(); + await executor.dedupStarted; + await eventually(() => executor.discoveryCalls === 4 && executor.runningDiscovery === 2); + executor.releaseDedup(); + + const terminal = await coordinator.wait(undefined, 5_000); + assert.equal(rejectedCancellations.size, 2, "the redundant discoveries reached the failing cancellation write"); + assert.equal(terminal?.status, "succeeded", terminal?.error); + assert.equal(terminal.terminalReason, "saturated"); + assert.equal(store.failCalls, 0); + assert.equal(store.finishCalls.length, 1); + assert.equal(store.finishCalls[0].reason, "saturated"); + assert.equal(executor.discoveryCalls, 4, "cancellation persistence failures must not dispatch replacement reviews after saturation"); + assert.equal(executor.dedupCalls, 1, "cancellation persistence failures must not restart reduction after saturation"); + assert.equal(executor.runningDiscovery, 0); + assert.equal(completed.length, 1); + assert.equal(completed[0].coverage.completeness, "complete"); + const acceptedReducer = [...store.workers.values()].find((worker) => ( + worker.kind === "dedup" && worker.status === "succeeded" + )); + assert.deepEqual( + completed[0], + JSON.parse(await readFile(acceptedReducer.resultManifestPath, "utf8")), + "the accepted aggregate still reaches publication when redundant cancellation writes fail", + ); + } + async function testPublicationUsesAcceptedReducerSnapshot() { const fixture = await fixtureRun({ workers: 1, subagents: 0, stopAfterNoNew: 1, maxDiscoveryRuns: 1 }); const store = new FakeStore(fixture.run); @@ -134,7 +175,8 @@ export async function testDeepScanPublication({ assert.equal(completed[0].coverage.completeness, "complete"); } - await testSaturationPublishesFindingAcceptedDuringCancellation(); + await testSaturationOmitsWorkerAcceptedDuringCancellation(); await testSuccessfulDeepCoverageIgnoresWorkerAndReducerReviewStatus(); + await testSaturationIgnoresDiscoveryCancellationWriteFailure(); await testPublicationUsesAcceptedReducerSnapshot(); } diff --git a/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs b/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs index 4c8c383a9..0574595c6 100644 --- a/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs +++ b/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs @@ -791,7 +791,7 @@ async function testDiscoveryDeadlineWithoutAcceptedWorkersPublishesEmptyResults( assert.deepEqual(JSON.parse(await readFile(terminal.manifestPath, "utf8")).findings, []); } -async function testSaturationDoesNotHideTerminalWorkerFailure() { +async function testSaturationIgnoresWorkerFailureSettledAfterStop() { const fixture = await fixtureRun({ workers: 3, subagents: 0, stopAfterNoNew: 2, maxDiscoveryRuns: 3 }); const store = new FakeStore(fixture.run); store.blockDiscoveryFailure = true; @@ -815,21 +815,24 @@ async function testSaturationDoesNotHideTerminalWorkerFailure() { await Promise.all([executor.dedupStarted, store.discoveryFailureBlocked.promise]); executor.releaseDedup(); - await eventually(() => coordinator.snapshot().noNewStreak === 2); + await eventually(() => executor.dedupSignal?.aborted === true); store.releaseDiscoveryFailure(); const terminal = await coordinator.wait(undefined, 5_000); - assert.equal(terminal?.status, "failed"); - assert.equal(terminal?.terminalReason, undefined); - assert.equal(completedDrafts.length, 0); - assert.match(terminal.error, /fixture configuration failure/); + assert.equal(terminal?.status, "succeeded", terminal?.error); + assert.equal(terminal?.terminalReason, "saturated"); + assert.equal(completedDrafts.length, 1); + assert.equal(completedDrafts[0].coverage.completeness, "complete"); + assert.deepEqual(completedDrafts[0].findings, []); const failedWorker = [...store.workers.values()].find((worker) => ( worker.status === "failed" && path.basename(path.dirname(worker.promptPath)) === "discovery-0003" )); assert.ok(failedWorker); assert.equal(failedWorker.status, "failed"); - assert.equal(store.failCalls, 1); - assert.equal(store.finishCalls.length, 0); + assert.equal(store.failCalls, 0); + assert.equal(store.finishCalls.length, 1); + assert.equal(executor.discoveryCalls, 3); + assert.equal(executor.dedupCalls, 1); } async function testSettledReducerIsNotStarvedByDiscoveryBacklog() { @@ -3965,7 +3968,7 @@ try { await testSaturationPreservesFindingAlreadyBuffered(); await testDeepScanPublication({ fixtureRun, FakeStore, FakeExecutor, DeepScanCoordinator, deferred, - immediateClock, eventually, standardScanDraft, + immediateClock, eventually, }); await testDirectReducerCannotDropAcceptedFinding(); await testSaturationDrainsBufferedAndCancelsInflight(); @@ -3975,7 +3978,7 @@ try { await testDiscoveryDeadlineWithoutAcceptedWorkersReturnsPartialEvidence(); await testDiscoveryDeadlineBeforeWorkerDispatchReturnsPartialEvidence(); await testDiscoveryDeadlineWithoutAcceptedWorkersPublishesEmptyResults(); - await testSaturationDoesNotHideTerminalWorkerFailure(); + await testSaturationIgnoresWorkerFailureSettledAfterStop(); await testSettledReducerIsNotStarvedByDiscoveryBacklog(); await testSingletonHardCapReduction(); await testExhaustedRetryFailsScan(); diff --git a/plugins/codex-security/scripts/deep_scan_workbench.py b/plugins/codex-security/scripts/deep_scan_workbench.py index 80f679524..799ccea43 100644 --- a/plugins/codex-security/scripts/deep_scan_workbench.py +++ b/plugins/codex-security/scripts/deep_scan_workbench.py @@ -1877,6 +1877,7 @@ def finish_deep_scan_locked( """ SELECT 1 FROM deep_scan_workers AS failed WHERE failed.scan_id = ? AND failed.status = 'failed' + AND (? != 'saturated' OR failed.kind != 'discovery') AND ( failed.kind != 'dedup' OR NOT EXISTS ( @@ -1902,10 +1903,14 @@ def finish_deep_scan_locked( ) LIMIT 1 """, - (scan_id,), + (scan_id, args.terminal_reason), ).fetchone() if failed_worker is not None and not failure_capped: raise SystemExit("Deep Scan cannot finish after a worker has failed.") + if args.terminal_reason == "saturated": + # The coordinator has stopped discovery. Its remaining workers cannot + # override that outcome if their own cancellation writes failed. + cancel_active_workers(connection, scan_id, now()) active_worker = connection.execute( """ SELECT 1 FROM deep_scan_workers diff --git a/plugins/codex-security/tests/test_deep_scan_stop_conditions.py b/plugins/codex-security/tests/test_deep_scan_stop_conditions.py new file mode 100644 index 000000000..5871eeedc --- /dev/null +++ b/plugins/codex-security/tests/test_deep_scan_stop_conditions.py @@ -0,0 +1,150 @@ +from __future__ import annotations + +import json +from argparse import Namespace + +import pytest +from test_deep_scan_successful_publication import ( + add_worker, + assert_published_aggregate, + complete, + publication_scan, +) + + +@pytest.fixture +def saturated_scan(publication_scan, workbench_db): + scan = publication_scan() + completed_result = add_worker(workbench_db, scan) + completed_result.write_text( + json.dumps( + { + "scanId": scan.scan_id, + "complete": True, + "findings": scan.findings, + "coverage": scan.coverage, + } + ) + ) + reducer_result = add_worker(workbench_db, scan) + reducer_result.write_bytes(completed_result.read_bytes()) + with workbench_db: + workbench_db.execute( + "UPDATE deep_scan_workers SET completion_sequence = 1 WHERE id = ?", + (completed_result.parent.name,), + ) + workbench_db.execute( + "UPDATE deep_scan_workers SET kind = 'dedup', merge_state = 'none' WHERE id = ?", + (reducer_result.parent.name,), + ) + workbench_db.execute( + "INSERT INTO deep_scan_dedup_inputs " + "(scan_id, dedup_worker_id, discovery_worker_id, input_order) VALUES (?, ?, ?, 0)", + (scan.scan_id, reducer_result.parent.name, completed_result.parent.name), + ) + workbench_db.execute( + "UPDATE deep_scan_runs SET status = 'running', phase = 'discovery', " + "consecutive_no_new = stop_after_no_new, completion_sequence = 1, " + "max_discovery_runs = 3, discovery_runs_dispatched = 1, " + "manifest_path = NULL, terminal_reason = NULL, completed_at = NULL WHERE scan_id = ?", + (scan.scan_id,), + ) + scan.completed_worker_id = completed_result.parent.name + scan.reducer_id = reducer_result.parent.name + return scan + + +def finish(workbench_api, connection, scan, *, terminal_reason="saturated"): + return workbench_api["deep_scan"].finish_deep_scan( + connection, + Namespace( + scan_id=scan.scan_id, + terminal_reason=terminal_reason, + manifest_path=str(scan.scan_dir / "scan-manifest.json"), + staged_manifest_path=None, + omitted_worker_id=[], + ), + )["deepScan"] + + +def test_saturated_finish_cancels_unfinished_discovery_without_using_its_drafts( + workbench_api, workbench_db, saturated_scan +): + scan = saturated_scan + unfinished = [] + for status in ("queued", "running"): + result = add_worker(workbench_db, scan, status=status) + result.write_text("{unfinished worker draft") + unfinished.append(result) + + finished = finish(workbench_api, workbench_db, scan) + + assert finished["status"] == "succeeded" + assert finished["terminalReason"] == "saturated" + workers = {worker["id"]: worker for worker in finished["workers"]} + assert workers[scan.completed_worker_id]["status"] == "succeeded" + assert workers[scan.reducer_id]["status"] == "succeeded" + for result in unfinished: + worker = workers[result.parent.name] + assert worker["status"] == "canceled" + assert worker["completedAt"] is not None + assert worker["completionSequence"] is None + assert worker["mergeState"] == "none" + assert result.read_text() == "{unfinished worker draft" + + complete(workbench_api, workbench_db, scan) + assert_published_aggregate(scan) + + +def test_saturated_finish_retains_failed_discovery_diagnostic( + workbench_api, workbench_db, saturated_scan +): + scan = saturated_scan + result = add_worker(workbench_db, scan, status="failed") + error = "Worker stopped while persisting its result." + with workbench_db: + workbench_db.execute( + "UPDATE deep_scan_workers SET error_message = ? WHERE id = ?", + (error, result.parent.name), + ) + + finished = finish(workbench_api, workbench_db, scan) + + assert finished["status"] == "succeeded" + failed_worker = next( + worker for worker in finished["workers"] if worker["id"] == result.parent.name + ) + assert failed_worker["status"] == "failed" + assert failed_worker["error"] == error + complete(workbench_api, workbench_db, scan) + assert_published_aggregate(scan) + + +@pytest.mark.parametrize( + ("terminal_reason", "kind"), + [("saturated", "setup"), ("saturated", "dedup"), ("capped", "discovery")], + ids=["saturated-setup", "saturated-reducer", "capped-discovery"], +) +def test_finish_preserves_other_worker_failure_checks( + workbench_api, workbench_db, saturated_scan, terminal_reason, kind +): + scan = saturated_scan + result = add_worker(workbench_db, scan, status="failed") + with workbench_db: + workbench_db.execute( + "UPDATE deep_scan_workers SET kind = ? WHERE id = ?", (kind, result.parent.name) + ) + if terminal_reason == "capped": + workbench_db.execute( + "UPDATE deep_scan_runs SET discovery_runs_dispatched = max_discovery_runs " + "WHERE scan_id = ?", + (scan.scan_id,), + ) + + with pytest.raises(SystemExit, match="after a worker has failed"): + finish(workbench_api, workbench_db, scan, terminal_reason=terminal_reason) + + run = workbench_db.execute( + "SELECT status, terminal_reason FROM deep_scan_runs WHERE scan_id = ?", (scan.scan_id,) + ).fetchone() + assert tuple(run) == ("running", None) diff --git a/plugins/codex-security/tests/test_workbench_deep_scan.py b/plugins/codex-security/tests/test_workbench_deep_scan.py index b8111a6f4..f7cf02a00 100644 --- a/plugins/codex-security/tests/test_workbench_deep_scan.py +++ b/plugins/codex-security/tests/test_workbench_deep_scan.py @@ -3944,20 +3944,6 @@ def test_discovery_buffer_prefix_dedup_and_saturation_are_transactional( manifest = scan_dir / "artifacts" / "deep_discovery" / "coordinator-manifest.json" manifest.write_text("{}\n") - active_rejected = run_workbench( - state_dir, - "finish-deep-scan", - "--scan-id", - scan_id, - "--terminal-reason", - "saturated", - "--manifest-path", - str(manifest), - environment=deep_environment(codex_home), - check=False, - ) - assert "while workers are active" in str(active_rejected["stderr"]) - late_result.write_text("{}\n") accepted_late = upsert_worker( state_dir, From 98c677d9313e802dc7e82ffd0e4c34e898b47e34 Mon Sep 17 00:00:00 2001 From: Dane Schneider <269480009+daneschneider-oai@users.noreply.github.com> Date: Mon, 31 Aug 2026 15:14:40 -0700 Subject: [PATCH 03/10] fix(deep-scan): derive publication inventory metadata --- plugins/codex-security/scripts/finalize_scan_contract.py | 7 ++----- .../codex-security/tests/test_finalize_scan_contract.py | 7 ++----- .../tests/test_workbench_completion_binding.py | 8 +++----- sdk/typescript/tests-ts/runtime.test.ts | 7 ++++--- 4 files changed, 11 insertions(+), 18 deletions(-) diff --git a/plugins/codex-security/scripts/finalize_scan_contract.py b/plugins/codex-security/scripts/finalize_scan_contract.py index e82dc36f4..ea9982a6d 100644 --- a/plugins/codex-security/scripts/finalize_scan_contract.py +++ b/plugins/codex-security/scripts/finalize_scan_contract.py @@ -1252,12 +1252,9 @@ def _normalize_unsealed_deep_repository_inventory_strategy( *, expected_coverage_mode: str | None, ) -> None: - """Normalize the old Deep workflow label to the ordinary repository inventory.""" + """Derive the inventory label from the selected Deep repository mode.""" - if ( - expected_coverage_mode == "deep_repository" - and coverage.get("inventoryStrategy") == "deep_repository_repeated_discovery" - ): + if expected_coverage_mode == "deep_repository": coverage["inventoryStrategy"] = "repository" diff --git a/plugins/codex-security/tests/test_finalize_scan_contract.py b/plugins/codex-security/tests/test_finalize_scan_contract.py index c9a50e501..9cedd21ad 100644 --- a/plugins/codex-security/tests/test_finalize_scan_contract.py +++ b/plugins/codex-security/tests/test_finalize_scan_contract.py @@ -618,7 +618,7 @@ def test_finalize_rejects_deep_inventory_strategy_alias_outside_deep(self) -> No expected_coverage_mode=expected_mode, ) - def test_finalize_rejects_unknown_deep_inventory_strategy_alias(self) -> None: + def test_finalize_rejects_unknown_inventory_without_selected_deep_mode(self) -> None: self.coverage["mode"] = "deep_repository" self.coverage["inventoryStrategy"] = "deep_repository_repeated_discovery_v2" self.write_scan() @@ -627,10 +627,7 @@ def test_finalize_rejects_unknown_deep_inventory_strategy_alias(self) -> None: FINALIZER.ContractError, "coverage.schema.inventoryStrategy: unsupported value", ): - FINALIZER.finalize_scan( - self.scan_dir, - expected_coverage_mode="deep_repository", - ) + FINALIZER.finalize_scan(self.scan_dir) def test_finalize_rejects_sealed_deep_inventory_strategy_alias(self) -> None: self.coverage["mode"] = "deep_repository" diff --git a/plugins/codex-security/tests/test_workbench_completion_binding.py b/plugins/codex-security/tests/test_workbench_completion_binding.py index 632a4f0e8..5eed724c6 100644 --- a/plugins/codex-security/tests/test_workbench_completion_binding.py +++ b/plugins/codex-security/tests/test_workbench_completion_binding.py @@ -462,7 +462,7 @@ def test_completion_keeps_recoverable_prewrite_failures_resumable( assert completed["findingCount"] == 1 -def test_deep_completion_recovers_malformed_inventory_without_dropping_findings( +def test_deep_completion_derives_inventory_without_downgrading_coverage( tmp_path: Path, ) -> None: for index, inventory in enumerate((None, "", "invalid_strategy")): @@ -481,13 +481,11 @@ def test_deep_completion_recovers_malformed_inventory_without_dropping_findings( assert completed["scan"]["progress"]["status"] == "complete" assert completed["scan"]["findingCount"] == 1 - assert completed["scan"]["warnings"] == [ - "Recovered malformed Deep Scan inventory strategy; marked coverage as partial." - ] + assert completed["scan"]["warnings"] == [] sealed_coverage = json.loads(coverage_path.read_text()) assert sealed_coverage["mode"] == "deep_repository" assert sealed_coverage["inventoryStrategy"] == "repository" - assert sealed_coverage["completeness"] == "partial" + assert sealed_coverage["completeness"] == "complete" assert len(json.loads((scan_dir / "findings.json").read_text())["findings"]) == 1 assert (scan_dir / "report.md").is_file() diff --git a/sdk/typescript/tests-ts/runtime.test.ts b/sdk/typescript/tests-ts/runtime.test.ts index 07c142f1f..d086a6295 100644 --- a/sdk/typescript/tests-ts/runtime.test.ts +++ b/sdk/typescript/tests-ts/runtime.test.ts @@ -294,9 +294,10 @@ describe("plugin runtime preparation", () => { ), ); const runtime = brotliDecompressSync(Buffer.concat(parts)).toString("utf8"); - const source = /function buildFindings\(findings\) \{[\s\S]*?\n\}/u.exec( - runtime, - )?.[0]; + const source = + /function buildFindings\(findings, mode\) \{[\s\S]*?\n\}/u.exec( + runtime, + )?.[0]; expect(source).toBeDefined(); const buildFindings = new Function( "semanticIdentifier", From 79808cb1c35941c22bb20baa8aab2e00b44e49eb Mon Sep 17 00:00:00 2001 From: Dane Schneider <269480009+daneschneider-oai@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:05:43 -0700 Subject: [PATCH 04/10] fix(deep-scan): simplify finding collision suffixes --- .../mcp-app/src/artifact-scan-draft.ts | 14 ++---- .../tests/test_artifact_scan_draft.mjs | 50 +++++++++++++++---- 2 files changed, 44 insertions(+), 20 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts b/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts index 2e9e1469a..bdc750dce 100644 --- a/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts +++ b/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts @@ -1459,21 +1459,15 @@ function buildFindings(findings: JsonObject[], mode?: string): JsonObject[] { return finding; } const identity = finding.identity as JsonObject; - const locations = (finding.locations as JsonObject[]).map((location) => JSON.stringify([ - location.path, location.startLine, location.endLine ?? location.startLine, - ])).sort(); - const digest = createHash("sha256") - .update(JSON.stringify([finding.ruleId, identity, locations])) - .digest("hex").slice(0, 16); - const baseInstance = `${identity.instance ?? "saved"}-${digest}`; + const baseInstance = identity.instance ?? "saved"; let suffix = 2; const distinct: JsonObject & { identity: JsonObject } = { - ...finding, identity: { ...identity, instance: baseInstance }, + ...finding, identity: { ...identity }, }; - while (reserved.has(scanFindingIdentity(distinct)) || used.has(scanFindingIdentity(distinct))) { + do { distinct.identity.instance = `${baseInstance}-${suffix}`; suffix += 1; - } + } while (reserved.has(scanFindingIdentity(distinct)) || used.has(scanFindingIdentity(distinct))); const provenance = finding.provenance as JsonObject; distinct.provenance = { ...provenance, diff --git a/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs b/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs index 429c3b6b7..acfbbd904 100644 --- a/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs +++ b/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs @@ -2309,13 +2309,14 @@ try { "duplicate stable instance sources remain collisions for finalization", ); - const collisionFindings = ["src/upload.py", "src/import.py"].map((location) => ({ + const collisionFindings = ["src/upload.py", "src/import.py", "src/restore.py"].map((location) => ({ ...finding, locations: [{ path: location, startLine: 41, endLine: 44 }], provenance: { source: "local_plugin" }, extensions: {}, })); const authoredCollisionIdentity = { anchor: "shared-archive-review" }; + const reservedCollisionIdentity = { ...authoredCollisionIdentity, instance: "parser" }; const collisionCases = [ { label: "authored", @@ -2327,6 +2328,24 @@ try { findings: collisionFindings, originalIdentity: { anchor: "unsafe-archive-extraction", instance: "unsafe-archive-extraction" }, }, + { + label: "authored with reserved suffixes", + findings: [ + ...collisionFindings.map((item) => ({ ...item, identity: reservedCollisionIdentity })), + ...collisionFindings.slice(0, 2).map((item, index) => ({ + ...item, + identity: { ...reservedCollisionIdentity, instance: `parser-${index + 2}` }, + })), + ], + originalIdentity: reservedCollisionIdentity, + expectedIdentities: [ + reservedCollisionIdentity, + { ...reservedCollisionIdentity, instance: "parser-4" }, + { ...reservedCollisionIdentity, instance: "parser-5" }, + { ...reservedCollisionIdentity, instance: "parser-2" }, + { ...reservedCollisionIdentity, instance: "parser-3" }, + ], + }, ]; for (const collisionCase of collisionCases) { const collisionInput = { ...input, findings: collisionCase.findings }; @@ -2334,7 +2353,7 @@ try { const standardFindings = (await readJson(root, "findings.json")).findings; assert.deepEqual( standardFindings.map((item) => item.identity), - [collisionCase.originalIdentity, collisionCase.originalIdentity], + collisionCase.findings.map((item) => item.identity ?? collisionCase.originalIdentity), `${collisionCase.label} identity collisions retain the existing Standard shape`, ); assert.deepEqual( @@ -2346,14 +2365,25 @@ try { await recordFreshScanDraft(deepIdentityContext, collisionInput); const deepFindings = (await readJson(root, "findings.json")).findings; const deepIdentities = deepFindings.map((item) => item.identity); - assert.equal(deepFindings.length, 2, `${collisionCase.label} identity collisions must retain both Deep findings`); - assert.equal(new Set(deepIdentities.map((identity) => JSON.stringify(identity))).size, 2); - assert.deepEqual(deepFindings[0].identity, collisionCase.originalIdentity); - assert.deepEqual(deepFindings[0].provenance, collisionCase.findings[0].provenance); - assert.deepEqual(deepFindings[1].provenance, { - ...collisionCase.findings[1].provenance, - preservedIdentity: collisionCase.originalIdentity, - }); + assert.equal(deepFindings.length, collisionCase.findings.length, `${collisionCase.label} identity collisions must retain every Deep finding`); + assert.deepEqual( + deepIdentities, + collisionCase.expectedIdentities ?? collisionCase.findings.map((_, index) => ( + index === 0 ? collisionCase.originalIdentity : { + ...collisionCase.originalIdentity, + instance: `${collisionCase.originalIdentity.instance ?? "saved"}-${index + 1}`, + } + )), + `${collisionCase.label} collisions receive successive numeric suffixes without replacing authored identities`, + ); + assert.deepEqual( + deepFindings.map((item) => item.provenance), + collisionCase.findings.map((item, index) => ( + index === 1 || index === 2 ? { + ...item.provenance, preservedIdentity: collisionCase.originalIdentity, + } : item.provenance + )), + ); assert.deepEqual( deepFindings.map((item) => item.locations), collisionCase.findings.map((item) => item.locations), From 48de9a56b482f33773e0551a75889b4de6797d16 Mon Sep 17 00:00:00 2001 From: Dane Schneider <269480009+daneschneider-oai@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:09:15 -0700 Subject: [PATCH 05/10] docs(deep-scan): clarify finding ID collision comment --- plugins/codex-security/mcp-app/src/artifact-scan-draft.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts b/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts index bdc750dce..bc0a8153d 100644 --- a/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts +++ b/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts @@ -1448,8 +1448,8 @@ function buildFindings(findings: JsonObject[], mode?: string): JsonObject[] { }); if (mode !== "deep") return identified; - // Independent workers can choose the same semantic identity for different - // findings. Assign distinct canonical instances without discarding evidence. + // Keep both findings when workers reuse an ID. + // Add a numeric suffix to make each ID unique. const reserved = new Set(identified.map(scanFindingIdentity)); const used = new Set(); return identified.map((finding) => { From 45f764d564efb0da0e32430c4b8b48e52503d913 Mon Sep 17 00:00:00 2001 From: Dane Schneider <269480009+daneschneider-oai@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:18:01 -0700 Subject: [PATCH 06/10] fix(deep-scan): derive coverage completeness in the host --- .../mcp-app/src/artifact-deep-reducer.ts | 19 ++-- .../src/deep-scan/artifact-validation.ts | 28 ++++-- .../mcp-app/src/deep-scan/worker-runner.ts | 2 +- .../src/server/compact-artifact-tools.ts | 2 +- .../mcp-app/templates/deep-scan/dedup.md | 4 +- .../tests/test_artifact_deep_reducer.mjs | 32 ++++--- .../tests/test_compact_artifact_server.mjs | 7 ++ .../test_deep_scan_artifact_validation.mjs | 85 +++++++++++++----- .../tests/test_deep_scan_coordinator.mjs | 4 +- .../schemas/tools/deep-reducer.schema.json | 49 +++++++++- .../tests/test_scan_contract_examples.py | 89 +++++++++++++++---- 11 files changed, 245 insertions(+), 76 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts b/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts index 4782fc485..e14c5fb08 100644 --- a/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts +++ b/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts @@ -21,7 +21,7 @@ import { writeJsonAtomic, type DeepScanArtifacts } from "./deep-scan/artifacts.js"; -import { reconcileDeepReduction } from "./deep-scan/artifact-validation.js"; +import { completedDeepScanCoverage, reconcileDeepReduction } from "./deep-scan/artifact-validation.js"; const schemaDocuments = [ commonSchema, @@ -107,7 +107,11 @@ export async function recordCodexSecurityDeepReduction( }> { return withLogicalReducerErrors(context, async () => { const bound = bindDeepReducer(context); - let reduction = parseScanDraft(input as ScanDraftInput); + const submitted = deepReductionInputSchema.parse(input); + let reduction = parseScanDraft({ + ...submitted, + coverage: completedDeepScanCoverage(submitted.coverage), + }); if (reduction.complete === false) throw new Error("Deep reduction is only a checkpoint, not a complete result."); const inputs = await getCodexSecurityDeepReducerInputs(context); const expectedScanId = bound.scanId @@ -171,18 +175,23 @@ async function readPreviousReduction( return parseStoredScanDraft( await readJsonObject(previousReducerResultPath), "The previous accepted Deep reduction", - bound.scanId + bound.scanId, + true ); } function parseStoredScanDraft( value: Record, label: string, - expectedScanId?: string + expectedScanId?: string, + reducer = false ): ScanDraftInput { let parsed: ScanDraftInput; try { - parsed = parsePersistedScanDraft(value); + parsed = parsePersistedScanDraft(reducer ? { + ...value, + coverage: completedDeepScanCoverage(value.coverage as Record), + } : value); } catch (error) { const detail = error instanceof Error ? error.message : String(error); throw new Error(label + " has an invalid Standard scan result: " + detail, { cause: error }); diff --git a/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts b/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts index 8fabff02f..bc40ea34a 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts @@ -56,7 +56,8 @@ export async function validateReducerArtifacts(input: { let result = parseStoredScanDraft( await readJsonObject(resultPath), reducerId, - expectedScanId + expectedScanId, + true ); if (result.complete === false) throw new Error("Deep reduction wrote only a checkpoint; its audit is not complete."); @@ -66,7 +67,8 @@ export async function validateReducerArtifacts(input: { previous = parseStoredScanDraft( await readJsonObject(previousReducerResultPath), "Previous successful reducer", - result.scanId + result.scanId, + true ); } @@ -155,13 +157,19 @@ export function reconcileDeepReduction( return result; } -function completedDeepScanCoverage(coverage: Record): Record { +export function completedDeepScanCoverage(coverage: Record): Record { + if (!coverage || typeof coverage !== "object" || Array.isArray(coverage)) return coverage; + const surfaces = coverage.surfaces; return { ...coverage, completeness: "complete", - surfaces: (coverage.surfaces as Record[]).filter( - (surface) => surface.disposition !== "needs_follow_up", - ).map(({ receiptRefs: _workerReceipts, ...surface }) => surface), + surfaces: Array.isArray(surfaces) ? surfaces.filter( + (surface) => surface?.disposition !== "needs_follow_up", + ).map((surface) => { + if (!surface || typeof surface !== "object" || Array.isArray(surface)) return surface; + const { receiptRefs: _workerReceipts, ...reviewed } = surface; + return reviewed; + }) : surfaces, deferred: [], }; } @@ -253,11 +261,15 @@ export function validateRetainedFindings( function parseStoredScanDraft( value: Record, label: string, - expectedScanId?: string + expectedScanId?: string, + reducer = false ): ScanDraftInput { let parsed: ScanDraftInput; try { - parsed = parsePersistedScanDraft(value); + parsed = parsePersistedScanDraft(reducer ? { + ...value, + coverage: completedDeepScanCoverage(value.coverage as Record), + } : value); } catch (error) { const detail = error instanceof Error ? error.message : String(error); throw new Error(label + " returned an invalid Standard scan result: " + detail, { diff --git a/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts b/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts index c9c32bf67..8507ef120 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts @@ -799,7 +799,7 @@ function standardScanCompletionContinuation(attempt: number): string { function reducerCompletionContinuation(attempt: number): string { return [ `Continue the existing Deep Scan reducer after attempt ${attempt} ended without its required result.`, - "Use your existing Standard scan analysis and call record_codex_security_deep_reduction({ scanId, findings, coverage, threatModel?, scope? }).", + "Submit the aggregate with record_codex_security_deep_reduction({ scanId, findings, coverage: { surfaces, explicitExclusions, openQuestions? }, threatModel?, scope? }).", "If the tool rejects the arguments, use its error to correct them and retry the call until it succeeds.", "Do not end your turn, write the result directly, or call the tool again after it succeeds." ].join("\n"); diff --git a/plugins/codex-security/mcp-app/src/server/compact-artifact-tools.ts b/plugins/codex-security/mcp-app/src/server/compact-artifact-tools.ts index f9a3dbe1c..58c3947b6 100644 --- a/plugins/codex-security/mcp-app/src/server/compact-artifact-tools.ts +++ b/plugins/codex-security/mcp-app/src/server/compact-artifact-tools.ts @@ -280,7 +280,7 @@ export function registerCompactWorkerArtifactTools( registerCompactTool(server, { name: "record_codex_security_deep_reduction", title: "Record Codex Security Deep Reduction", - description: "Record this reducer's complete aggregated Standard scan result.", + description: "Record the merged findings and review notes for this Deep scan.", inputSchema: deepReductionInputSchema, readOnly: false, handler: async (value) => recordCodexSecurityDeepReduction( diff --git a/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md b/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md index 443bdd566..dc6e7b6ad 100644 --- a/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md +++ b/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md @@ -16,6 +16,6 @@ For a valid merge, synthesize one stronger finding while preserving every materi Account for every input finding using the host-supplied `provenance.sourceFindingIds`. Copy the refs for retained findings; union them for a valid merge, preserving previous refs. Never invent, omit, or reuse a ref across independent output findings. The host retains original source payloads and rejects unaccounted input. Identity collisions do not establish that findings are duplicates. -Summarize the reviewed surfaces, explicit exclusions, open questions, threat-model context, and optional scope in the aggregate. Individual workers are independent reviews: do not carry their partial or unknown coverage, deferred work, needs_follow_up surfaces, or receiptRefs into parent coverage. Their original results retain those observations and receipt references. Use complete coverage with an empty deferred array for the accepted aggregate; include only actually reviewed surface dispositions, never relabel unresolved work as reviewed. The host owns the Deep Scan outcome. You cannot resolve or reject a source finding without inspecting code, which is outside this reducer's role. +Summarize reviewed surfaces, explicit exclusions, and open questions in `coverage`, with threat-model context and scope as needed. The host sets coverage completeness and handles worker follow-ups and receipt links. You cannot resolve or reject a source finding without inspecting code, which is outside this reducer's role. -Call `record_codex_security_deep_reduction({ scanId, findings, coverage, threatModel?, scope? })` with one complete Standard semantic result until it succeeds; correct a reported validation error and retry in the same conversation. After the first successful call, do not call it again. The host derives convergence and worker attribution from its existing state. +Call `record_codex_security_deep_reduction({ scanId, findings, coverage: { surfaces, explicitExclusions, openQuestions? }, threatModel?, scope? })` until it succeeds; correct a reported validation error and retry in the same conversation. After the first successful call, do not call it again. The host derives convergence and worker attribution from its existing state. diff --git a/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs b/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs index 547e7b81e..18fbbf500 100644 --- a/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs +++ b/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs @@ -22,11 +22,12 @@ const { ); const scanId = "7fc17317-9594-49e0-b06a-d72fd7e14bba"; -const validDraft = draft([]); +const validDraft = draft([], { coverage: { surfaces: [], explicitExclusions: [] } }); assert.equal(deepReducerInputsInputSchema.safeParse({}).success, true); assert.equal(deepReducerInputsInputSchema.safeParse({ path: "/tmp" }).success, false); assert.equal(deepReductionInputSchema.safeParse(validDraft).success, true); +assert.equal(deepReductionInputSchema.safeParse(draft([])).success, true, "legacy coverage fields remain accepted"); assert.equal( deepReductionInputSchema.safeParse({ ...validDraft, resultPath: "/tmp" }).success, false @@ -111,17 +112,6 @@ try { assert.equal(JSON.stringify(inputs).includes(root), false); assert.equal(JSON.stringify(inputs).includes("result.json"), false); - const invalidCoverage = { - ...first.result, - coverage: { - ...first.result.coverage, - deferred: [{ reason: "A related path needs follow-up." }] - } - }; - await assert.rejects( - recordCodexSecurityDeepReduction(context, invalidCoverage), - /complete coverage cannot contain deferred/ - ); await assert.rejects( readFile(path.join(outputRoot, "result.json"), "utf8"), { code: "ENOENT" } @@ -132,12 +122,14 @@ try { ); const merged = draft([shared, independent], { + coverage: { surfaces: [], explicitExclusions: [] }, threatModel: { summary: "Requests reach shared and independent code." }, scope: { summary: "Shared and independent request handling." } }); const outcome = await recordCodexSecurityDeepReduction(context, merged); const mergedWithSources = { ...merged, + coverage: { ...merged.coverage, completeness: "complete", deferred: [] }, findings: [ retainedFinding(shared, [{ id: "worker-001:0", finding: shared }, { id: "worker-002:0", finding: shared }]), retainedFinding(independent, [{ id: "worker-002:1", finding: independent }]), @@ -195,7 +187,7 @@ try { ); const submittedCoverage = { - completeness: "partial", + completeness: "complete", surfaces: [ { label: "Response rendering", disposition: "no_issue_found", notes: "All outputs use contextual encoding.", receiptRefs: ["artifacts/missing-worker-receipt.md"] }, @@ -218,7 +210,7 @@ try { ], deferred: [], }, - "the host retains reviewed descriptions without linking missing optional worker receipts", + "the host projects legacy coverage before validation while retaining reviewed descriptions", ); assert.deepEqual( await Promise.all(coverageWorkers.map((worker) => readFile(worker.resultPath, "utf8"))), @@ -310,15 +302,21 @@ try { const enrichedPrevious = structuredClone(mergedWithSources); enrichedPrevious.findings[0].summary = "The earlier reduction established an additional reachable output route."; enrichedPrevious.findings[0].validation = { summary: "Both output routes bypass the same encoding control." }; - enrichedPrevious.coverage = rejectedCoverage; + enrichedPrevious.coverage = { ...rejectedCoverage, completeness: "complete" }; const previousArtifact = JSON.stringify(enrichedPrevious); await writeFile(path.join(outputRoot, "result.json"), previousArtifact); + const normalizedPrevious = (await getCodexSecurityDeepReducerInputs(nextContext)).previous; + assert.equal(normalizedPrevious.coverage.completeness, "complete"); + assert.deepEqual(normalizedPrevious.coverage.deferred, []); + assert.deepEqual(normalizedPrevious.coverage.surfaces, [ + { label: "SQL route", disposition: "rejected", notes: "Parameterized queries prevent injection." }, + ]); await recordCodexSecurityDeepReduction(nextContext, merged); const preservedEnrichment = JSON.parse(await readFile(path.join(nextOutputRoot, "result.json"), "utf8")); assert.deepEqual( preservedEnrichment.coverage, - merged.coverage, - "a previous reducer's partial coverage is not inherited by a subsequent accepted reduction", + mergedWithSources.coverage, + "a previous reducer's coverage decision is not inherited by a subsequent accepted reduction", ); assert.equal( await readFile(path.join(outputRoot, "result.json"), "utf8"), diff --git a/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs b/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs index d1af7e3da..136e8e707 100644 --- a/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs +++ b/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs @@ -1155,6 +1155,13 @@ async function testReducerWorkerToolList(bundle) { ); if (tool.name === "record_codex_security_deep_reduction") { assert.equal(tool.inputSchema.required?.includes("scanId"), true); + const coverage = tool.inputSchema.properties.coverage; + assert.deepEqual(coverage.required, ["surfaces", "explicitExclusions"]); + assert.equal(Object.hasOwn(coverage.properties, "openQuestions"), true); + for (const field of ["completeness", "deferred"]) { + assert.equal(Object.hasOwn(coverage.properties, field), false, + `The reducer must not be asked to choose coverage.${field}.`); + } } for (const forbidden of [ "path", diff --git a/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs b/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs index 2144f4837..22ff92a29 100644 --- a/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs +++ b/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs @@ -146,6 +146,19 @@ async function testDiscoveryValidation(root) { /complete coverage cannot contain deferred/ ); + await writeResult(worker.resultPath, { + ...result, + coverage: { + ...result.coverage, + surfaces: [{ label: "Unfinished Standard review", disposition: "needs_follow_up" }], + }, + }); + await assert.rejects( + validateDiscoveryArtifacts(artifacts, worker.resultPath, scanId), + /complete coverage cannot contain needs_follow_up/, + "reducer coverage normalization must not relax Standard discovery semantics", + ); + await writeResult(worker.resultPath, { ...result, findings: [{ @@ -295,9 +308,12 @@ async function testReducerValidation(root) { await writeResult(resultPath, draft([firstFinding], { coverage: { completeness: "complete", - surfaces: [resolvedCoverageSurface], + surfaces: [ + resolvedCoverageSurface, + { label: "Legacy reducer follow-up", disposition: "needs_follow_up" }, + ], explicitExclusions: [], - deferred: [], + deferred: [{ reason: "A legacy reducer copied pending worker review work." }], }, })); const validatedCoverage = await validateReducerArtifacts({ @@ -439,24 +455,30 @@ async function testReducerValidation(root) { openQuestions: ["Should a future review include generated handlers?"], }, }); - await writeResult(resultPath, legacyPartial); - const legacyArtifact = await readFile(resultPath, "utf8"); - const resumed = await validate(); - assert.equal(resumed.newFindings, 1); - assert.deepEqual(resumed.result, { - ...legacyPartial, - coverage: { - ...legacyPartial.coverage, - completeness: "complete", - surfaces: [resolvedCoverageSurface], - deferred: [], - }, - }); - assert.equal( - await readFile(resultPath, "utf8"), - legacyArtifact, - "resuming a legacy partial reducer returns normalized coverage without rewriting its original artifact", - ); + for (const completeness of ["partial", "complete"]) { + const legacyReducer = { + ...legacyPartial, + coverage: { ...legacyPartial.coverage, completeness }, + }; + await writeResult(resultPath, legacyReducer); + const legacyArtifact = await readFile(resultPath, "utf8"); + const resumed = await validate(); + assert.equal(resumed.newFindings, 1); + assert.deepEqual(resumed.result, { + ...legacyReducer, + coverage: { + ...legacyReducer.coverage, + completeness: "complete", + surfaces: [resolvedCoverageSurface], + deferred: [], + }, + }); + assert.equal( + await readFile(resultPath, "utf8"), + legacyArtifact, + `resuming legacy ${completeness} reducer coverage normalizes pending work without rewriting its original artifact`, + ); + } await writeResult(resultPath, { ...legacyPartial, complete: false }); await assert.rejects(validate(), /only a checkpoint|not complete/); @@ -476,10 +498,20 @@ async function testReducerValidation(root) { "result.json" ); await mkdir(path.dirname(previousReducerResultPath), { recursive: true }); - await writeResult(previousReducerResultPath, draft([firstFinding])); + await writeResult(previousReducerResultPath, { + ...legacyPartial, + coverage: { ...legacyPartial.coverage, completeness: "complete" }, + }); + const previousArtifact = await readFile(previousReducerResultPath, "utf8"); assert.equal( (await validate(previousReducerResultPath)).newFindings, - 1 + 1, + "previous reducer coverage is normalized before validation and does not change finding novelty", + ); + assert.equal( + await readFile(previousReducerResultPath, "utf8"), + previousArtifact, + "reading a previous reducer must not rewrite its original coverage", ); const renamedTitle = { ...firstFinding, title: "Stronger explanation of the same finding." }; @@ -580,7 +612,9 @@ async function testEmptyDiscoveryAndReduction(root) { const artifactDir = path.join(artifacts.dedupRoot, "dedup-empty", "output"); const resultPath = path.join(artifactDir, "result.json"); await mkdir(artifactDir, { recursive: true }); - await writeResult(resultPath, draft([])); + await writeResult(resultPath, draft([], { + coverage: { surfaces: [], explicitExclusions: [] }, + })); const result = await validateReducerArtifacts({ artifacts, artifactDir, @@ -588,6 +622,11 @@ async function testEmptyDiscoveryAndReduction(root) { reducerId: "dedup-empty" }); assert.equal(result.newFindings, 0); + assert.deepEqual( + result.result, + draft([]), + "reducers can omit coverage completion fields owned by the host", + ); } async function createLayout(scanDir) { diff --git a/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs b/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs index 0574595c6..8a0f252ce 100644 --- a/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs +++ b/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs @@ -1747,7 +1747,7 @@ async function testMissingReducerResultResumesExistingThread() { assert.equal(new Set(executor.dedupThreadIds).size, 1); assert.match( executor.dedupContinuationPrompts[1] ?? "", - /record_codex_security_deep_reduction\(\{ scanId, findings, coverage, threatModel\?, scope\? \}\)/ + /record_codex_security_deep_reduction\(\{ scanId, findings, coverage: \{ surfaces, explicitExclusions, openQuestions\? \}, threatModel\?, scope\? \}\)/ ); assert.doesNotMatch(executor.dedupContinuationPrompts[1] ?? "", /\{ candidates, merges \}/); assert.match(executor.dedupContinuationPrompts[1] ?? "", /retry the call until it succeeds/); @@ -3852,7 +3852,7 @@ async function writeDedupArtifacts(request, consumedOverride, options = {}) { if (options.canonicalCandidateId && draft.findings.length > 0) { draft.findings[0].provenance.candidateId = options.canonicalCandidateId; } - if (consumedOverride || options.omitLastWorkerSource) draft.coverage.completeness = "invalid"; + if (consumedOverride || options.omitLastWorkerSource) draft.coverage.surfaces = "invalid"; if (options.dropLastFinding) draft.findings.pop(); const resultPath = path.join(artifactContext.root, "result.json"); await mkdir(path.dirname(resultPath), { recursive: true }); diff --git a/plugins/codex-security/schemas/tools/deep-reducer.schema.json b/plugins/codex-security/schemas/tools/deep-reducer.schema.json index d9ca75ecb..87dd5896b 100644 --- a/plugins/codex-security/schemas/tools/deep-reducer.schema.json +++ b/plugins/codex-security/schemas/tools/deep-reducer.schema.json @@ -9,7 +9,54 @@ "additionalProperties": false }, "reductionInput": { - "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/scanDraftInput" + "type": "object", + "properties": { + "complete": { + "type": "boolean", + "description": "Omit or set true when submitting the finished aggregate." + }, + "scanId": { + "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/scanId" + }, + "handoffClaimToken": { + "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/handoffClaimToken" + }, + "scope": { + "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/scope" + }, + "threatModel": { + "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/threatModel" + }, + "findings": { + "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/scanDraftInput/properties/findings" + }, + "coverage": { + "type": "object", + "description": "Reviewed surfaces, exclusions, and open questions. The host sets coverage completeness.", + "properties": { + "surfaces": { + "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/coverage/properties/surfaces" + }, + "explicitExclusions": { + "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/coverage/properties/explicitExclusions" + }, + "openQuestions": { + "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/coverage/properties/openQuestions" + } + }, + "required": [ + "surfaces", + "explicitExclusions" + ], + "additionalProperties": true + } + }, + "required": [ + "scanId", + "findings", + "coverage" + ], + "additionalProperties": false } } } diff --git a/plugins/codex-security/tests/test_scan_contract_examples.py b/plugins/codex-security/tests/test_scan_contract_examples.py index 26e43286b..f16fbce4f 100644 --- a/plugins/codex-security/tests/test_scan_contract_examples.py +++ b/plugins/codex-security/tests/test_scan_contract_examples.py @@ -54,10 +54,15 @@ def test_schemas_are_valid_draft_2020_12(self) -> None: with self.subTest(schema=schema_path.name): Draft202012Validator.check_schema(read_json(schema_path)) - def test_deep_reducer_schema_preserves_complete_standard_results(self) -> None: + def test_deep_reducer_schema_accepts_factual_coverage_and_standard_findings(self) -> None: common_schema = read_json(SCHEMA_DIR / "definitions" / "artifact-common.schema.json") scan_draft_schema = read_json(SCHEMA_DIR / "tools" / "scan-draft.schema.json") reducer_schema = read_json(SCHEMA_DIR / "tools" / "deep-reducer.schema.json") + reducer_coverage = reducer_schema["$defs"]["reductionInput"]["properties"]["coverage"] + for field in ("completeness", "deferred"): + self.assertNotIn(field, reducer_coverage["properties"]) + self.assertNotIn(field, reducer_coverage["required"]) + self.assertEqual(set(reducer_coverage["required"]), {"surfaces", "explicitExclusions"}) registry = Registry().with_resources( (schema["$id"], Resource.from_contents(schema)) for schema in (common_schema, scan_draft_schema) @@ -78,10 +83,8 @@ def test_deep_reducer_schema_preserves_complete_standard_results(self) -> None: "provenance": {"source": "local_plugin"}, } coverage = { - "completeness": "complete", - "surfaces": [], - "explicitExclusions": [], - "deferred": [], + "surfaces": [{"label": "HTTP responses", "disposition": "reported"}], + "explicitExclusions": [{"pattern": "docs/", "reason": "Documentation only."}], } Draft202012Validator.check_schema(reducer_schema) @@ -105,6 +108,65 @@ def test_deep_reducer_schema_preserves_complete_standard_results(self) -> None: } ) validator.validate({**request, "findings": []}) + validator.validate( + { + **request, + "coverage": { + **coverage, + "openQuestions": [{"question": "Which deployments expose this route?"}], + }, + } + ) + for completeness in ("complete", "partial", "unknown"): + with self.subTest(legacy_completeness=completeness): + validator.validate( + { + **request, + "coverage": { + **coverage, + "completeness": completeness, + "deferred": [{"id": "old-follow-up", "reason": "Legacy review note."}], + }, + } + ) + + standard_validator = Draft202012Validator( + scan_draft_schema, registry=registry, format_checker=FormatChecker() + ) + standard_request = { + **request, + "coverage": {**coverage, "completeness": "complete", "deferred": []}, + } + standard_validator.validate(standard_request) + self.assertFalse(standard_validator.is_valid(request)) + for missing_field in ("completeness", "deferred"): + with self.subTest(standard_missing_coverage_field=missing_field): + self.assertFalse( + standard_validator.is_valid( + { + **standard_request, + "coverage": { + field: value + for field, value in standard_request["coverage"].items() + if field != missing_field + }, + } + ) + ) + for missing_field in ("surfaces", "explicitExclusions"): + with self.subTest(deep_missing_coverage_field=missing_field): + self.assertFalse( + validator.is_valid( + { + **request, + "coverage": { + field: value + for field, value in coverage.items() + if field != missing_field + }, + } + ) + ) for extra_field in ( "source_worker_id", @@ -128,18 +190,13 @@ def test_deep_reducer_schema_preserves_complete_standard_results(self) -> None: "provenance", ): with self.subTest(missing_field=missing_field): + incomplete_finding = { + field: value for field, value in finding.items() if field != missing_field + } + self.assertFalse(validator.is_valid({**request, "findings": [incomplete_finding]})) self.assertFalse( - validator.is_valid( - { - **request, - "findings": [ - { - field: value - for field, value in finding.items() - if field != missing_field - } - ], - } + standard_validator.is_valid( + {**standard_request, "findings": [incomplete_finding]} ) ) From 08b668cfd78ed8ca2b3cd9a7d4aa248419e14e5a Mon Sep 17 00:00:00 2001 From: Dane Schneider <269480009+daneschneider-oai@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:56:47 -0700 Subject: [PATCH 07/10] fix(deep-scan): remove coverage from aggregation --- .../mcp-app/src/artifact-deep-reducer.ts | 48 +++--- .../mcp-app/src/artifact-scan-draft.ts | 4 +- .../src/deep-scan/artifact-validation.ts | 87 +++++----- .../mcp-app/src/deep-scan/coordinator.ts | 20 ++- .../mcp-app/src/deep-scan/worker-runner.ts | 7 +- .../src/server/compact-artifact-tools.ts | 6 +- .../mcp-app/templates/deep-scan/dedup.md | 6 +- .../tests/deep_scan_publication_cases.mjs | 5 +- .../tests/test_artifact_deep_reducer.mjs | 162 ++++++++++-------- .../tests/test_compact_artifact_server.mjs | 14 +- .../test_deep_scan_artifact_validation.mjs | 61 +++---- .../tests/test_deep_scan_coordinator.mjs | 7 +- .../tests/test_deep_scan_templates.mjs | 3 + .../codex-security/references/final-report.md | 2 +- .../references/scan-artifacts.md | 2 + .../references/scan-contract.md | 6 +- .../schemas/tools/deep-reducer.schema.json | 23 +-- .../scripts/report_projection.py | 2 +- .../scripts/workbench_saved_results.py | 12 +- .../skills/deep-security-scan/SKILL.md | 2 + .../test_deep_scan_successful_publication.py | 123 +++++++++---- .../tests/test_scan_contract_examples.py | 75 ++++---- .../deep-scan-reducer-recovery.test.ts | 28 ++- 23 files changed, 384 insertions(+), 321 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts b/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts index e14c5fb08..f5d9f2f14 100644 --- a/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts +++ b/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts @@ -6,9 +6,7 @@ import scanDraftSchema from "../../schemas/tools/scan-draft.schema.json"; import type { ArtifactContext, DeepReducerContext } from "./artifact-context.js"; import { parsePersistedScanDraft, - parseScanDraft, saveScanDraftCheckpoint, - type ScanDraftInput } from "./artifact-scan-draft.js"; import { loadArtifactZodSchema, @@ -21,7 +19,11 @@ import { writeJsonAtomic, type DeepScanArtifacts } from "./deep-scan/artifacts.js"; -import { completedDeepScanCoverage, reconcileDeepReduction } from "./deep-scan/artifact-validation.js"; +import { + parseDeepReduction, + reconcileDeepReduction, + type DeepReductionInput, +} from "./deep-scan/artifact-validation.js"; const schemaDocuments = [ commonSchema, @@ -39,14 +41,14 @@ export const deepReductionInputSchema = loadArtifactZodSchema( schemaDocuments, reducerSchema.$id, "reductionInput" -) as ZodType; +) as ZodType; interface DeepReducerInputs { discoveries: { workerId: string; - result: ScanDraftInput; + result: DeepReductionInput; }[]; - previous: ScanDraftInput | null; + previous: DeepReductionInput | null; } interface BoundReducer { @@ -56,7 +58,7 @@ interface BoundReducer { scanId?: string; } -/** Return complete Standard results without exposing their artifact locations. */ +/** Return assigned findings and context without exposing worker artifact locations. */ export async function getCodexSecurityDeepReducerInputs( context: ArtifactContext ): Promise { @@ -67,7 +69,8 @@ export async function getCodexSecurityDeepReducerInputs( const result = parseStoredScanDraft( await readJsonObject(worker.resultPath), "Accepted Standard worker " + worker.id, - bound.scanId + bound.scanId, + parsePersistedScanDraft ); if (result.complete === false) throw new Error("An assigned Standard worker wrote only a checkpoint, not a complete result."); result.findings = result.findings.map((finding, index) => ({ @@ -77,7 +80,8 @@ export async function getCodexSecurityDeepReducerInputs( sourceFindingIds: [`${worker.id}:${index}`], }, })); - return { workerId: worker.id, result }; + const { coverage: _coverage, ...reduction } = result; + return { workerId: worker.id, result: reduction }; })); const previous = await readPreviousReduction(bound); const scanId = bound.scanId ?? previous?.scanId ?? discoveries[0]?.result.scanId; @@ -97,7 +101,7 @@ export async function getCodexSecurityDeepReducerInputs( }); } -/** Validate and durably replace this reducer's complete semantic Standard result. */ +/** Validate and durably replace this reducer's complete semantic result. */ export async function recordCodexSecurityDeepReduction( context: ArtifactContext, input: unknown @@ -108,10 +112,7 @@ export async function recordCodexSecurityDeepReduction( return withLogicalReducerErrors(context, async () => { const bound = bindDeepReducer(context); const submitted = deepReductionInputSchema.parse(input); - let reduction = parseScanDraft({ - ...submitted, - coverage: completedDeepScanCoverage(submitted.coverage), - }); + let reduction = parseDeepReduction(submitted); if (reduction.complete === false) throw new Error("Deep reduction is only a checkpoint, not a complete result."); const inputs = await getCodexSecurityDeepReducerInputs(context); const expectedScanId = bound.scanId @@ -168,7 +169,7 @@ function bindDeepReducer(context: ArtifactContext): BoundReducer { async function readPreviousReduction( bound: BoundReducer -): Promise { +): Promise { const { previousReducerResultPath } = bound.state; if (!previousReducerResultPath) return null; await requireRegularFile(previousReducerResultPath, bound.artifacts.dedupRoot); @@ -176,22 +177,19 @@ async function readPreviousReduction( await readJsonObject(previousReducerResultPath), "The previous accepted Deep reduction", bound.scanId, - true + (value) => parseDeepReduction(value, true) ); } -function parseStoredScanDraft( +function parseStoredScanDraft( value: Record, label: string, - expectedScanId?: string, - reducer = false -): ScanDraftInput { - let parsed: ScanDraftInput; + expectedScanId: string | undefined, + parse: (input: Record) => Result +): Result { + let parsed: Result; try { - parsed = parsePersistedScanDraft(reducer ? { - ...value, - coverage: completedDeepScanCoverage(value.coverage as Record), - } : value); + parsed = parse(value); } catch (error) { const detail = error instanceof Error ? error.message : String(error); throw new Error(label + " has an invalid Standard scan result: " + detail, { cause: error }); diff --git a/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts b/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts index bc0a8153d..65a879f14 100644 --- a/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts +++ b/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts @@ -273,7 +273,7 @@ export async function recordCodexSecurityWorkerScanDraft( /** Keep the semantic input before any replaceable worker or canonical artifact. */ export async function saveScanDraftCheckpoint( context: ArtifactContext, - input: ScanDraftInput, + input: Omit, updateHead = true, ): Promise { const { handoffClaimToken: _claim, ...snapshot } = input; @@ -519,7 +519,7 @@ async function readCurrentCheckpoints( return checkpoints.map(({ input }) => input); } -function scanDraftCheckpointName(input: ScanDraftInput): string { +function scanDraftCheckpointName(input: Omit): string { const { handoffClaimToken: _claim, ...snapshot } = input; return createHash("sha256").update(JSON.stringify(snapshot)).digest("hex") + ".json"; } diff --git a/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts b/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts index bc40ea34a..e3d0ac4e5 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts @@ -1,6 +1,7 @@ import { isDeepStrictEqual } from "node:util"; import { parsePersistedScanDraft, + parseScanDraft, preserveFindingDetails, saveScanDraftCheckpoint, scanFindingIdentity, @@ -9,14 +10,37 @@ import { import { readJsonObject, requireRegularFile, writeJsonAtomic } from "./artifacts.js"; import type { DeepScanArtifacts } from "./artifacts.js"; +export type DeepReductionInput = Omit; + export interface DeepReductionSources { - discoveries: { workerId: string; result: ScanDraftInput }[]; - previous: ScanDraftInput | null; + discoveries: { workerId: string; result: DeepReductionInput }[]; + previous: DeepReductionInput | null; } export interface ReducerArtifactValidation { newFindings: number; - result: ScanDraftInput; + result: DeepReductionInput; +} + +/** Reuse Standard finding validation without admitting legacy reducer coverage. */ +export function parseDeepReduction( + input: Record, + persisted = false, +): DeepReductionInput { + const { coverage: _legacyCoverage, ...reduction } = input; + const standard = { + ...reduction, + coverage: { + completeness: "complete", + surfaces: [], + explicitExclusions: [], + deferred: [], + }, + }; + const { coverage: _coverage, ...parsed } = persisted + ? parsePersistedScanDraft(standard) + : parseScanDraft(standard as unknown as ScanDraftInput); + return parsed; } /** Admit exactly the complete semantic result written by an ordinary Standard scan. */ @@ -29,7 +53,8 @@ export async function validateDiscoveryArtifacts( const result = parseStoredScanDraft( await readJsonObject(resultPath), "Standard scan worker", - expectedScanId + expectedScanId, + parsePersistedScanDraft ); if (result.complete === false) throw new Error("Standard scan worker wrote only a checkpoint; its audit is not complete."); return result; @@ -57,7 +82,7 @@ export async function validateReducerArtifacts(input: { await readJsonObject(resultPath), reducerId, expectedScanId, - true + (value) => parseDeepReduction(value, true) ); if (result.complete === false) throw new Error("Deep reduction wrote only a checkpoint; its audit is not complete."); @@ -68,7 +93,7 @@ export async function validateReducerArtifacts(input: { await readJsonObject(previousReducerResultPath), "Previous successful reducer", result.scanId, - true + (value) => parseDeepReduction(value, true) ); } @@ -78,7 +103,6 @@ export async function validateReducerArtifacts(input: { await writeJsonAtomic(resultPath, result); } else { validateRetainedFindings(result, [], previous); - result.coverage = completedDeepScanCoverage(result.coverage); } const previousFindingIds = new Set((previous?.findings ?? []).map(scanFindingIdentity)); return { @@ -91,10 +115,10 @@ export async function validateReducerArtifacts(input: { /** Reconcile a reducer output against the immutable inputs captured before dispatch. */ export function reconcileDeepReduction( - input: ScanDraftInput, + input: DeepReductionInput, discoveries: DeepReductionSources["discoveries"], - previous: ScanDraftInput | null, -): ScanDraftInput { + previous: DeepReductionInput | null, +): DeepReductionInput { const result = structuredClone(input); if (result.complete === false) throw new Error("Deep reduction is only a checkpoint, not a complete result."); for (const source of [...discoveries.map((discovery) => discovery.result), ...(previous ? [previous] : [])]) { @@ -121,9 +145,6 @@ export function reconcileDeepReduction( } } retainSourceFindings(result, { discoveries, previous }); - // Each worker is an independent review. Its unfinished review observations - // remain in its own saved result rather than becoming parent scan work. - result.coverage = completedDeepScanCoverage(result.coverage); if (result.threatModel === undefined) { const sourceModels = [ ...discoveries.map((discovery) => discovery.result.threatModel), @@ -157,23 +178,6 @@ export function reconcileDeepReduction( return result; } -export function completedDeepScanCoverage(coverage: Record): Record { - if (!coverage || typeof coverage !== "object" || Array.isArray(coverage)) return coverage; - const surfaces = coverage.surfaces; - return { - ...coverage, - completeness: "complete", - surfaces: Array.isArray(surfaces) ? surfaces.filter( - (surface) => surface?.disposition !== "needs_follow_up", - ).map((surface) => { - if (!surface || typeof surface !== "object" || Array.isArray(surface)) return surface; - const { receiptRefs: _workerReceipts, ...reviewed } = surface; - return reviewed; - }) : surfaces, - deferred: [], - }; -} - function findingSourceIds(finding: Record): string[] { const provenance = finding.provenance as Record; const ids = provenance.sourceFindingIds; @@ -187,7 +191,7 @@ function findingSourceIds(finding: Record): string[] { )); } -function retainSourceFindings(result: ScanDraftInput, inputs: DeepReductionSources): void { +function retainSourceFindings(result: DeepReductionInput, inputs: DeepReductionSources): void { type Finding = Record; const sources = new Map(); for (const discovery of inputs.discoveries) { @@ -235,9 +239,9 @@ function retainSourceFindings(result: ScanDraftInput, inputs: DeepReductionSourc /** Preserve previously accepted identities and never discard every reported finding. */ export function validateRetainedFindings( - result: ScanDraftInput, - sources: ScanDraftInput[], - previous?: ScanDraftInput + result: DeepReductionInput, + sources: DeepReductionInput[], + previous?: DeepReductionInput ): void { if ( result.findings.length === 0 @@ -258,18 +262,15 @@ export function validateRetainedFindings( } } -function parseStoredScanDraft( +function parseStoredScanDraft( value: Record, label: string, - expectedScanId?: string, - reducer = false -): ScanDraftInput { - let parsed: ScanDraftInput; + expectedScanId: string | undefined, + parse: (input: Record) => Result +): Result { + let parsed: Result; try { - parsed = parsePersistedScanDraft(reducer ? { - ...value, - coverage: completedDeepScanCoverage(value.coverage as Record), - } : value); + parsed = parse(value); } catch (error) { const detail = error instanceof Error ? error.message : String(error); throw new Error(label + " returned an invalid Standard scan result: " + detail, { diff --git a/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts b/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts index 89b7050f5..83e60dd76 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts @@ -5,7 +5,7 @@ import { createDeepScanArtifacts, ensureDeepScanDirectories } from "./artifacts.js"; -import { validateDiscoveryArtifacts, validateReducerArtifacts } from "./artifact-validation.js"; +import { validateDiscoveryArtifacts, validateReducerArtifacts, type DeepReductionInput } from "./artifact-validation.js"; import { scanDraftInputSchema, type ScanDraftInput @@ -57,7 +57,7 @@ interface SchedulerResult { accepted: AcceptedDiscovery[]; mergedWorkerIds: string[]; reducers: AcceptedReducer[]; - result?: ScanDraftInput; + result?: DeepReductionInput; } type CoordinatorPhase = "setup" | "discovery" | "terminal"; @@ -293,7 +293,17 @@ export class DeepScanCoordinator { if (this.canceled || this.externallyFailed) return; this.phase = "terminal"; const draft = schedulerResult.result - ? structuredClone(schedulerResult.result) + ? { + ...structuredClone(schedulerResult.result), + // Keep the saved coverage contract for existing readers. Deep + // completion comes from the coordinator, not worker observations. + coverage: { + completeness: "complete", + surfaces: [], + explicitExclusions: [], + deferred: [] + } + } : scanDraftInputSchema.parse({ scanId: this.state.scanId, findings: [], @@ -989,11 +999,11 @@ export class DeepScanCoordinator { private async recoverCompletedReducers( discoveries: AcceptedDiscovery[] - ): Promise<{ reducers: AcceptedReducer[]; result?: ScanDraftInput }> { + ): Promise<{ reducers: AcceptedReducer[]; result?: DeepReductionInput }> { const discoveriesById = new Map(discoveries.map((worker) => [worker.id, worker])); const inputs = this.state.persistedDedupInputs ?? []; const outcomes: AcceptedReducer[] = []; - let latestResult: ScanDraftInput | undefined; + let latestResult: DeepReductionInput | undefined; const completedReducers = (this.state.persistedWorkers ?? []) .filter((worker) => worker.kind === "dedup" && worker.status === "succeeded") .sort((left, right) => ( diff --git a/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts b/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts index 8507ef120..82f119979 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts @@ -2,12 +2,11 @@ import { createHash } from "node:crypto"; import { promises as fs } from "node:fs"; import { dirname, join } from "node:path"; import { getCodexSecurityDeepReducerInputs } from "../artifact-deep-reducer.js"; -import type { ScanDraftInput } from "../artifact-scan-draft.js"; import { validateDiscoveryArtifacts, validateReducerArtifacts } from "./artifact-validation.js"; -import type { ReducerArtifactValidation } from "./artifact-validation.js"; +import type { DeepReductionInput, ReducerArtifactValidation } from "./artifact-validation.js"; import { archiveDirectory, discoveryArtifacts, @@ -63,7 +62,7 @@ export interface SuccessfulDedupOutcome { id: string; consumed: AcceptedDiscovery[]; resultPath: string; - result: ScanDraftInput; + result: DeepReductionInput; newFindings: number; attempt: number; threadId?: string; @@ -799,7 +798,7 @@ function standardScanCompletionContinuation(attempt: number): string { function reducerCompletionContinuation(attempt: number): string { return [ `Continue the existing Deep Scan reducer after attempt ${attempt} ended without its required result.`, - "Submit the aggregate with record_codex_security_deep_reduction({ scanId, findings, coverage: { surfaces, explicitExclusions, openQuestions? }, threatModel?, scope? }).", + "Submit the aggregate with record_codex_security_deep_reduction({ scanId, findings, threatModel?, scope? }).", "If the tool rejects the arguments, use its error to correct them and retry the call until it succeeds.", "Do not end your turn, write the result directly, or call the tool again after it succeeds." ].join("\n"); diff --git a/plugins/codex-security/mcp-app/src/server/compact-artifact-tools.ts b/plugins/codex-security/mcp-app/src/server/compact-artifact-tools.ts index 58c3947b6..69e931e76 100644 --- a/plugins/codex-security/mcp-app/src/server/compact-artifact-tools.ts +++ b/plugins/codex-security/mcp-app/src/server/compact-artifact-tools.ts @@ -271,7 +271,7 @@ export function registerCompactWorkerArtifactTools( registerCompactTool(server, { name: "get_codex_security_deep_reducer_inputs", title: "Get Codex Security Deep Reducer Inputs", - description: "Read the complete Standard scan results assigned to this reducer.", + description: "Read the assigned findings, context, and previous aggregate.", inputSchema: deepReducerInputsInputSchema, readOnly: true, handler: async () => getCodexSecurityDeepReducerInputs(context) @@ -280,12 +280,12 @@ export function registerCompactWorkerArtifactTools( registerCompactTool(server, { name: "record_codex_security_deep_reduction", title: "Record Codex Security Deep Reduction", - description: "Record the merged findings and review notes for this Deep scan.", + description: "Record the merged findings and context for this Deep scan.", inputSchema: deepReductionInputSchema, readOnly: false, handler: async (value) => recordCodexSecurityDeepReduction( context, - value as ScanDraftInput + value ) }); } diff --git a/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md b/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md index dc6e7b6ad..5f6271a29 100644 --- a/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md +++ b/plugins/codex-security/mcp-app/templates/deep-scan/dedup.md @@ -6,7 +6,7 @@ Use this exact reducer configuration: {{DEDUP_CONTEXT_JSON}} ``` -Call `get_codex_security_deep_reducer_inputs({})` to read each assigned worker's complete, already-validated Standard scan result and the previous aggregate, if any. The scan identity, findings, coverage, threat model, and scope already use the Standard semantic scan-draft contract. +Call `get_codex_security_deep_reducer_inputs({})` to read the findings and context from each assigned, already-validated Standard scan and the previous aggregate, if any. Read every finding and the previous aggregate. Merge only the same actionable root issue using remediation-subsumption: fixing the retained finding must also fix every absorbed finding. Preserve distinct reachable vulnerable instances, proof tuples, useful evidence, uncertainty, locations, provenance, severity, validation, attack paths, and remediation. @@ -16,6 +16,6 @@ For a valid merge, synthesize one stronger finding while preserving every materi Account for every input finding using the host-supplied `provenance.sourceFindingIds`. Copy the refs for retained findings; union them for a valid merge, preserving previous refs. Never invent, omit, or reuse a ref across independent output findings. The host retains original source payloads and rejects unaccounted input. Identity collisions do not establish that findings are duplicates. -Summarize reviewed surfaces, explicit exclusions, and open questions in `coverage`, with threat-model context and scope as needed. The host sets coverage completeness and handles worker follow-ups and receipt links. You cannot resolve or reject a source finding without inspecting code, which is outside this reducer's role. +Preserve the threat-model context and scope as needed. You cannot resolve or reject a source finding without inspecting code, which is outside this reducer's role. -Call `record_codex_security_deep_reduction({ scanId, findings, coverage: { surfaces, explicitExclusions, openQuestions? }, threatModel?, scope? })` until it succeeds; correct a reported validation error and retry in the same conversation. After the first successful call, do not call it again. The host derives convergence and worker attribution from its existing state. +Call `record_codex_security_deep_reduction({ scanId, findings, threatModel?, scope? })` until it succeeds; correct a reported validation error and retry in the same conversation. After the first successful call, do not call it again. The host derives convergence and worker attribution from its existing state. diff --git a/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs b/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs index 8ca603be1..11b62f5a8 100644 --- a/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs +++ b/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs @@ -88,7 +88,7 @@ export async function testDeepScanPublication({ assert.equal(terminal?.status, "succeeded", terminal?.error); assert.equal(completed.length, 1); assert.deepEqual(completed[0].coverage, { - completeness: "complete", surfaces: [reviewed], explicitExclusions: [], deferred: [], + completeness: "complete", surfaces: [], explicitExclusions: [], deferred: [], }); for (const worker of store.workers.values()) { if (worker.kind !== "discovery") continue; @@ -140,8 +140,9 @@ export async function testDeepScanPublication({ const acceptedReducer = [...store.workers.values()].find((worker) => ( worker.kind === "dedup" && worker.status === "succeeded" )); + const { coverage, ...publishedReduction } = completed[0]; assert.deepEqual( - completed[0], + publishedReduction, JSON.parse(await readFile(acceptedReducer.resultManifestPath, "utf8")), "the accepted aggregate still reaches publication when redundant cancellation writes fail", ); diff --git a/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs b/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs index 18fbbf500..208d76dc0 100644 --- a/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs +++ b/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs @@ -1,5 +1,5 @@ import assert from "node:assert/strict"; -import { mkdir, mkdtemp, readFile, realpath, rm, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, readFile, readdir, realpath, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import path from "node:path"; import { build } from "esbuild"; @@ -22,18 +22,19 @@ const { ); const scanId = "7fc17317-9594-49e0-b06a-d72fd7e14bba"; -const validDraft = draft([], { coverage: { surfaces: [], explicitExclusions: [] } }); +const validReduction = reduction([]); assert.equal(deepReducerInputsInputSchema.safeParse({}).success, true); assert.equal(deepReducerInputsInputSchema.safeParse({ path: "/tmp" }).success, false); -assert.equal(deepReductionInputSchema.safeParse(validDraft).success, true); -assert.equal(deepReductionInputSchema.safeParse(draft([])).success, true, "legacy coverage fields remain accepted"); +assert.equal(deepReductionInputSchema.safeParse(validReduction).success, true); +assert.equal(deepReductionInputSchema.safeParse(workerDraft([])).success, false, + "Deep reducer submissions no longer accept coverage"); assert.equal( - deepReductionInputSchema.safeParse({ ...validDraft, resultPath: "/tmp" }).success, + deepReductionInputSchema.safeParse({ ...validReduction, resultPath: "/tmp" }).success, false ); assert.equal( - deepReductionInputSchema.safeParse({ ...validDraft, consumedWorkerIds: ["spoofed"] }).success, + deepReductionInputSchema.safeParse({ ...validReduction, consumedWorkerIds: ["spoofed"] }).success, false ); assert.equal( @@ -59,7 +60,7 @@ try { workersRoot, label: "discovery-0001", id: "worker-001", - result: draft([shared], { + result: workerDraft([shared], { threatModel: { summary: "Requests may reach shared code." } }), completionSequence: 1 @@ -68,7 +69,7 @@ try { workersRoot, label: "discovery-0002", id: "worker-002", - result: draft([shared, independent], { + result: workerDraft([shared, independent], { scope: { summary: "Shared and independent request handling." } }), completionSequence: 2 @@ -88,20 +89,28 @@ try { const inputs = await getCodexSecurityDeepReducerInputs(context); await assert.rejects( - recordCodexSecurityDeepReduction(context, draft([], { complete: false })), + recordCodexSecurityDeepReduction(context, reduction([], { complete: false })), /only a checkpoint/, - "host-owned coverage must not accept an unfinished reducer result", + "a reducer submission must contain a complete result", ); await assert.rejects( - recordCodexSecurityDeepReduction(context, draft([shared])), + recordCodexSecurityDeepReduction(context, reduction([shared])), /unaccounted|discarded.*finding/, "a successful reduction must account for every fresh finding, not just one", ); await assert.rejects( - recordCodexSecurityDeepReduction(context, draft([shared, independent, finding("invented", "src/unreviewed.ts")])), + recordCodexSecurityDeepReduction(context, reduction([shared, independent, finding("invented", "src/unreviewed.ts")])), /no assigned source finding/, "a reducer cannot introduce an unvalidated finding outside its assigned sources", ); + await assert.rejects( + recordCodexSecurityDeepReduction(context, reduction([ + { ...shared, validation: { evidenceRefs: ["missing-evidence"] } }, + independent, + ], { complete: true })), + /evidenceRefs must refer/, + "live reducer submissions reject unknown evidence references instead of silently removing them", + ); assert.deepEqual(inputs, { discoveries: [ { workerId: first.id, result: withSourceRefs(first) }, @@ -117,19 +126,22 @@ try { { code: "ENOENT" } ); await assert.rejects( - recordCodexSecurityDeepReduction(context, draft([])), + readdir(path.join(outputRoot, "checkpoints")), + { code: "ENOENT" }, + "invalid reducer submissions do not save a checkpoint", + ); + await assert.rejects( + recordCodexSecurityDeepReduction(context, reduction([])), /discarded every accepted Standard scan finding/ ); - const merged = draft([shared, independent], { - coverage: { surfaces: [], explicitExclusions: [] }, + const merged = reduction([shared, independent], { threatModel: { summary: "Requests reach shared and independent code." }, scope: { summary: "Shared and independent request handling." } }); const outcome = await recordCodexSecurityDeepReduction(context, merged); const mergedWithSources = { ...merged, - coverage: { ...merged.coverage, completeness: "complete", deferred: [] }, findings: [ retainedFinding(shared, [{ id: "worker-001:0", finding: shared }, { id: "worker-002:0", finding: shared }]), retainedFinding(independent, [{ id: "worker-002:1", finding: independent }]), @@ -143,6 +155,13 @@ try { JSON.parse(await readFile(path.join(outputRoot, "result.json"), "utf8")), mergedWithSources ); + const checkpointNames = await readdir(path.join(outputRoot, "checkpoints")); + assert.equal(checkpointNames.length, 1); + assert.deepEqual( + JSON.parse(await readFile(path.join(outputRoot, "checkpoints", checkpointNames[0]), "utf8")), + mergedWithSources, + "reducer checkpoints retain the accepted findings and scope without coverage", + ); const rejectedCoverage = { completeness: "partial", @@ -157,11 +176,11 @@ try { }; const coverageWorker = await createWorker({ workersRoot, label: "discovery-coverage", id: "worker-coverage", - result: draft([], { coverage: rejectedCoverage }), completionSequence: 4, + result: workerDraft([], { coverage: rejectedCoverage }), completionSequence: 4, }); const unknownCoverageWorker = await createWorker({ workersRoot, label: "discovery-unknown", id: "worker-unknown", - result: draft([], { coverage: { ...draft([]).coverage, completeness: "unknown" } }), + result: workerDraft([], { coverage: { ...workerDraft([]).coverage, completeness: "unknown" } }), completionSequence: 5, }); const coverageWorkers = [coverageWorker, unknownCoverageWorker]; @@ -177,51 +196,24 @@ try { assert.deepEqual( (await getCodexSecurityDeepReducerInputs(coverageContext)).discoveries, coverageWorkers.map((worker) => ({ workerId: worker.id, result: withSourceRefs(worker) })), - "the reducer can still inspect each worker's original coverage evidence", + "worker coverage is absent from model-visible reducer inputs", ); - await recordCodexSecurityDeepReduction(coverageContext, draft([])); + await recordCodexSecurityDeepReduction(coverageContext, reduction([])); assert.deepEqual( - JSON.parse(await readFile(path.join(coverageRoot, "result.json"), "utf8")).coverage, - draft([]).coverage, - "accepted reductions do not inherit worker coverage even when every worker is partial or unknown", - ); - - const submittedCoverage = { - completeness: "complete", - surfaces: [ - { label: "Response rendering", disposition: "no_issue_found", notes: "All outputs use contextual encoding.", - receiptRefs: ["artifacts/missing-worker-receipt.md"] }, - { label: "Upload parsing", disposition: "rejected", notes: "Archive entries are not extracted." }, - { label: "Alternate handler", disposition: "needs_follow_up", notes: "A worker suggested another review." }, - ], - explicitExclusions: [{ pattern: "generated", reason: "Generated files are outside the requested scope." }], - deferred: [{ reason: "Repeat the alternate handler review.", paths: ["src/alternate.ts"] }], - openQuestions: ["Should future scans include generated handlers?"], - }; - await recordCodexSecurityDeepReduction(coverageContext, draft([], { coverage: submittedCoverage })); - assert.deepEqual( - JSON.parse(await readFile(path.join(coverageRoot, "result.json"), "utf8")).coverage, - { - ...submittedCoverage, - completeness: "complete", - surfaces: [ - { label: "Response rendering", disposition: "no_issue_found", notes: "All outputs use contextual encoding." }, - submittedCoverage.surfaces[1], - ], - deferred: [], - }, - "the host projects legacy coverage before validation while retaining reviewed descriptions", + JSON.parse(await readFile(path.join(coverageRoot, "result.json"), "utf8")), + reduction([]), + "accepted reductions omit coverage even when every worker is partial or unknown", ); assert.deepEqual( await Promise.all(coverageWorkers.map((worker) => readFile(worker.resultPath, "utf8"))), originalWorkerArtifacts, - "completing aggregate coverage must not rewrite raw worker evidence", + "reduction must not rewrite raw Standard worker coverage evidence", ); const collision = { ...independent, ruleId: shared.ruleId, identity: shared.identity }; const collisionWorker = await createWorker({ workersRoot, label: "discovery-collision", id: "worker-collision", - result: draft([shared, collision]), completionSequence: 5, + result: workerDraft([shared, collision]), completionSequence: 5, }); const collisionRoot = path.join(dedupRoot, "dedup-collision", "output"); await mkdir(collisionRoot, { recursive: true }); @@ -229,13 +221,13 @@ try { ...context, root: collisionRoot, deepReducer: { scanRoot, claimedWorkers: [collisionWorker] }, }; - await assert.rejects(recordCodexSecurityDeepReduction(collisionContext, draft([shared])), /ambiguous|unaccounted/); + await assert.rejects(recordCodexSecurityDeepReduction(collisionContext, reduction([shared])), /ambiguous|unaccounted/); const collisionInputs = await getCodexSecurityDeepReducerInputs(collisionContext); const sourceFindingIds = collisionInputs.discoveries[0].result.findings.flatMap( finding => finding.provenance.sourceFindingIds, ); assert.deepEqual(sourceFindingIds, ["worker-collision:0", "worker-collision:1"]); - await recordCodexSecurityDeepReduction(collisionContext, draft([{ + await recordCodexSecurityDeepReduction(collisionContext, reduction([{ ...shared, provenance: { ...shared.provenance, sourceFindingIds }, }])); const collisionOutput = JSON.parse(await readFile(path.join(collisionRoot, "result.json"), "utf8")); @@ -243,7 +235,7 @@ try { { id: "worker-collision:0", finding: shared }, { id: "worker-collision:1", finding: collision }, ]); - await assert.rejects(recordCodexSecurityDeepReduction(collisionContext, draft([{ + await assert.rejects(recordCodexSecurityDeepReduction(collisionContext, reduction([{ ...shared, provenance: { ...shared.provenance, sourceFindingIds: ["unassigned:0"] }, }])), /unknown source finding/); await assert.rejects( @@ -255,7 +247,7 @@ try { workersRoot, label: "discovery-0003", id: "worker-003", - result: draft([shared]), + result: workerDraft([shared]), completionSequence: 3 }); const nextOutputRoot = path.join(dedupRoot, "dedup-0002", "output"); @@ -277,7 +269,7 @@ try { previous: mergedWithSources }); await assert.rejects( - recordCodexSecurityDeepReduction(nextContext, draft([shared])), + recordCodexSecurityDeepReduction(nextContext, reduction([shared])), (error) => error.code === "merge_traceability_unstable_candidate_id" ); assert.deepEqual( @@ -302,21 +294,31 @@ try { const enrichedPrevious = structuredClone(mergedWithSources); enrichedPrevious.findings[0].summary = "The earlier reduction established an additional reachable output route."; enrichedPrevious.findings[0].validation = { summary: "Both output routes bypass the same encoding control." }; - enrichedPrevious.coverage = { ...rejectedCoverage, completeness: "complete" }; - const previousArtifact = JSON.stringify(enrichedPrevious); - await writeFile(path.join(outputRoot, "result.json"), previousArtifact); - const normalizedPrevious = (await getCodexSecurityDeepReducerInputs(nextContext)).previous; - assert.equal(normalizedPrevious.coverage.completeness, "complete"); - assert.deepEqual(normalizedPrevious.coverage.deferred, []); - assert.deepEqual(normalizedPrevious.coverage.surfaces, [ - { label: "SQL route", disposition: "rejected", notes: "Parameterized queries prevent injection." }, - ]); + for (const legacyCoverage of [ + rejectedCoverage, + { completeness: "outdated", surfaces: [null], explicitExclusions: false, deferred: 42 }, + "legacy coverage is no longer structured", + ]) { + const previousArtifact = JSON.stringify({ ...enrichedPrevious, coverage: legacyCoverage }); + await writeFile(path.join(outputRoot, "result.json"), previousArtifact); + assert.deepEqual( + (await getCodexSecurityDeepReducerInputs(nextContext)).previous, + enrichedPrevious, + "previous reducer coverage is ignored even when malformed; findings and scope remain intact", + ); + assert.equal( + await readFile(path.join(outputRoot, "result.json"), "utf8"), + previousArtifact, + "reading a previous reduction does not rewrite its legacy coverage", + ); + } + const previousArtifact = await readFile(path.join(outputRoot, "result.json"), "utf8"); await recordCodexSecurityDeepReduction(nextContext, merged); const preservedEnrichment = JSON.parse(await readFile(path.join(nextOutputRoot, "result.json"), "utf8")); - assert.deepEqual( - preservedEnrichment.coverage, - mergedWithSources.coverage, - "a previous reducer's coverage decision is not inherited by a subsequent accepted reduction", + assert.equal( + Object.hasOwn(preservedEnrichment, "coverage"), + false, + "a subsequent accepted reduction omits the previous reducer's legacy coverage", ); assert.equal( await readFile(path.join(outputRoot, "result.json"), "utf8"), @@ -359,9 +361,18 @@ try { await assert.rejects( getCodexSecurityDeepReducerInputs(context), /only a checkpoint/, - "host-owned coverage must not admit unfinished Standard worker results", + "unfinished Standard worker results are not reducer inputs", ); + for (const invalidCoverage of [undefined, { ...workerDraft([]).coverage, completeness: "outdated" }]) { + await writeFile(first.resultPath, JSON.stringify({ ...first.result, coverage: invalidCoverage })); + await assert.rejects( + getCodexSecurityDeepReducerInputs(context), + /coverage|completeness/, + "Standard worker coverage remains required and validated before projection", + ); + } + await writeFile(first.resultPath, "{invalid Standard scan\n"); await assert.rejects( getCodexSecurityDeepReducerInputs(context), @@ -394,7 +405,11 @@ async function createWorker({ workersRoot, label, id, result, completionSequence return { id, resultPath, completionSequence, result }; } -function draft(findings, extra = {}) { +function reduction(findings, extra = {}) { + return { scanId, findings, ...extra }; +} + +function workerDraft(findings, extra = {}) { return { scanId, findings, @@ -409,8 +424,9 @@ function draft(findings, extra = {}) { } function withSourceRefs(worker) { + const { coverage: _coverage, ...result } = worker.result; return { - ...worker.result, + ...result, findings: worker.result.findings.map((finding, index) => ({ ...finding, provenance: { ...finding.provenance, sourceFindingIds: [`${worker.id}:${index}`] }, diff --git a/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs b/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs index 136e8e707..153bfaf9f 100644 --- a/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs +++ b/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs @@ -1151,17 +1151,13 @@ async function testReducerWorkerToolList(bundle) { assert.equal( Object.hasOwn(tool.inputSchema.properties ?? {}, "scanId"), tool.name === "record_codex_security_deep_reduction", - `${tool.name} must expose scanId only when submitting its complete Standard draft.` + `${tool.name} must expose scanId only when submitting its complete reduction.` ); if (tool.name === "record_codex_security_deep_reduction") { - assert.equal(tool.inputSchema.required?.includes("scanId"), true); - const coverage = tool.inputSchema.properties.coverage; - assert.deepEqual(coverage.required, ["surfaces", "explicitExclusions"]); - assert.equal(Object.hasOwn(coverage.properties, "openQuestions"), true); - for (const field of ["completeness", "deferred"]) { - assert.equal(Object.hasOwn(coverage.properties, field), false, - `The reducer must not be asked to choose coverage.${field}.`); - } + assert.deepEqual(tool.inputSchema.required, ["scanId", "findings"]); + assert.equal(tool.inputSchema.additionalProperties, false); + assert.equal(Object.hasOwn(tool.inputSchema.properties, "coverage"), false, + "The reducer must not be asked to submit coverage."); } for (const forbidden of [ "path", diff --git a/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs b/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs index 22ff92a29..9147137bf 100644 --- a/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs +++ b/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs @@ -231,6 +231,7 @@ async function testReducerValidation(root) { admitted, "validation returns the same reconciled result that was accepted on disk", ); + assert.equal(Object.hasOwn(admitted, "coverage"), false); assert.deepEqual(admitted.findings[1].provenance.sourceFindingIds, ["worker-001:1"]); sources.previous = structuredClone(admitted); sources.previous.findings[0].summary = "Additional proof established by the previous reducer."; @@ -307,13 +308,9 @@ async function testReducerValidation(root) { }; await writeResult(resultPath, draft([firstFinding], { coverage: { - completeness: "complete", - surfaces: [ - resolvedCoverageSurface, - { label: "Legacy reducer follow-up", disposition: "needs_follow_up" }, - ], - explicitExclusions: [], - deferred: [{ reason: "A legacy reducer copied pending worker review work." }], + completeness: "invalid legacy value", + surfaces: null, + deferred: "invalid legacy collection", }, })); const validatedCoverage = await validateReducerArtifacts({ @@ -344,18 +341,14 @@ async function testReducerValidation(root) { previous: null, }, }, scanId); - const reconciledCoverage = JSON.parse(await readFile(resultPath, "utf8")).coverage; + const reconciledResult = JSON.parse(await readFile(resultPath, "utf8")); + assert.equal(Object.hasOwn(reconciledResult, "coverage"), false); assert.deepEqual( - reconciledCoverage, - { - completeness: "complete", - surfaces: [resolvedCoverageSurface], - explicitExclusions: [], - deferred: [], - }, - "the accepted aggregate retains its own coverage without inheriting worker review work", + validatedCoverage.result, + reconciledResult, + "accepted reducer results omit both malformed legacy coverage and worker coverage", ); - assert.deepEqual(validatedCoverage.result.coverage, reconciledCoverage); + assert.deepEqual(reconciledResult.findings[0].provenance.sourceFindingIds, ["worker-001:0"]); await writeResult(resultPath, draft([])); await assert.rejects( @@ -455,28 +448,24 @@ async function testReducerValidation(root) { openQuestions: ["Should a future review include generated handlers?"], }, }); - for (const completeness of ["partial", "complete"]) { + for (const [label, legacyCoverage] of [ + ["partial", legacyPartial.coverage], + ["complete with pending work", { ...legacyPartial.coverage, completeness: "complete" }], + ["malformed", null], + ]) { const legacyReducer = { ...legacyPartial, - coverage: { ...legacyPartial.coverage, completeness }, + coverage: legacyCoverage, }; await writeResult(resultPath, legacyReducer); const legacyArtifact = await readFile(resultPath, "utf8"); const resumed = await validate(); assert.equal(resumed.newFindings, 1); - assert.deepEqual(resumed.result, { - ...legacyReducer, - coverage: { - ...legacyReducer.coverage, - completeness: "complete", - surfaces: [resolvedCoverageSurface], - deferred: [], - }, - }); + assert.deepEqual(resumed.result, { scanId, findings: [firstFinding] }); assert.equal( await readFile(resultPath, "utf8"), legacyArtifact, - `resuming legacy ${completeness} reducer coverage normalizes pending work without rewriting its original artifact`, + `resuming a reducer with ${label} coverage ignores it without rewriting the original artifact`, ); } await writeResult(resultPath, { ...legacyPartial, complete: false }); @@ -486,7 +475,7 @@ async function testReducerValidation(root) { assert.equal((await validate()).newFindings, 0); await writeResult(resultPath, { ...draft([firstFinding]), resultPath: "/tmp/result.json" }); - await assert.rejects(validate(), /invalid Standard scan result/); + await assert.rejects(validate(), /resultPath/); await writeResult(resultPath, draft([firstFinding, secondFinding])); assert.equal((await validate()).newFindings, 2); @@ -500,13 +489,13 @@ async function testReducerValidation(root) { await mkdir(path.dirname(previousReducerResultPath), { recursive: true }); await writeResult(previousReducerResultPath, { ...legacyPartial, - coverage: { ...legacyPartial.coverage, completeness: "complete" }, + coverage: "malformed legacy coverage", }); const previousArtifact = await readFile(previousReducerResultPath, "utf8"); assert.equal( (await validate(previousReducerResultPath)).newFindings, 1, - "previous reducer coverage is normalized before validation and does not change finding novelty", + "malformed previous reducer coverage is ignored and does not change finding novelty", ); assert.equal( await readFile(previousReducerResultPath, "utf8"), @@ -612,9 +601,7 @@ async function testEmptyDiscoveryAndReduction(root) { const artifactDir = path.join(artifacts.dedupRoot, "dedup-empty", "output"); const resultPath = path.join(artifactDir, "result.json"); await mkdir(artifactDir, { recursive: true }); - await writeResult(resultPath, draft([], { - coverage: { surfaces: [], explicitExclusions: [] }, - })); + await writeResult(resultPath, { scanId, findings: [] }); const result = await validateReducerArtifacts({ artifacts, artifactDir, @@ -624,8 +611,8 @@ async function testEmptyDiscoveryAndReduction(root) { assert.equal(result.newFindings, 0); assert.deepEqual( result.result, - draft([]), - "reducers can omit coverage completion fields owned by the host", + { scanId, findings: [] }, + "reducers submit and return results without coverage", ); } diff --git a/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs b/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs index 8a0f252ce..0625bbbb1 100644 --- a/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs +++ b/plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs @@ -1747,7 +1747,7 @@ async function testMissingReducerResultResumesExistingThread() { assert.equal(new Set(executor.dedupThreadIds).size, 1); assert.match( executor.dedupContinuationPrompts[1] ?? "", - /record_codex_security_deep_reduction\(\{ scanId, findings, coverage: \{ surfaces, explicitExclusions, openQuestions\? \}, threatModel\?, scope\? \}\)/ + /record_codex_security_deep_reduction\(\{ scanId, findings, threatModel\?, scope\? \}\)/ ); assert.doesNotMatch(executor.dedupContinuationPrompts[1] ?? "", /\{ candidates, merges \}/); assert.match(executor.dedupContinuationPrompts[1] ?? "", /retry the call until it succeeds/); @@ -2161,7 +2161,7 @@ async function testReducerTraceabilityRetryNamesExactMissingSource() { ]); assert.doesNotMatch(basePrompt, /artifact_validation_failed/); assert.match(retryPrompt, /artifact_validation_failed/); - assert.match(retryPrompt, /coverage.*completeness/s); + assert.match(retryPrompt, /findings/s); assert.equal(context.claimedWorkerIds.includes(missingWorkerId), true); } @@ -3852,7 +3852,8 @@ async function writeDedupArtifacts(request, consumedOverride, options = {}) { if (options.canonicalCandidateId && draft.findings.length > 0) { draft.findings[0].provenance.candidateId = options.canonicalCandidateId; } - if (consumedOverride || options.omitLastWorkerSource) draft.coverage.surfaces = "invalid"; + delete draft.coverage; + if (consumedOverride || options.omitLastWorkerSource) draft.findings = "invalid"; if (options.dropLastFinding) draft.findings.pop(); const resultPath = path.join(artifactContext.root, "result.json"); await mkdir(path.dirname(resultPath), { recursive: true }); diff --git a/plugins/codex-security/mcp-app/tests/test_deep_scan_templates.mjs b/plugins/codex-security/mcp-app/tests/test_deep_scan_templates.mjs index b2c6930bb..86c2fc28f 100644 --- a/plugins/codex-security/mcp-app/tests/test_deep_scan_templates.mjs +++ b/plugins/codex-security/mcp-app/tests/test_deep_scan_templates.mjs @@ -25,6 +25,7 @@ const rendered = renderDiscoveryPrompt({ assert.doesNotMatch(rendered, /false_positive_feedback\.json/); assert.match(rendered, /preserve literal \{\{DISCOVERY_CONTEXT_JSON\}\} text/); assert.match(rendered, /record_codex_security_scan_draft/); +assert.match(rendered, /coverage\.deferred/); const discoveryContext = firstJsonBlock(rendered); assert.deepEqual(discoveryContext, { scanId: "a0d89285-66b7-4e4f-b51a-e21b93b7081b", @@ -68,6 +69,8 @@ const dedup = renderDedupPrompt({ }] }); const dedupContext = firstJsonBlock(dedup); +assert.doesNotMatch(dedup, /\bcoverage\b/i); +assert.match(dedup, /record_codex_security_deep_reduction\(\{ scanId, findings, threatModel\?, scope\? \}\)/); assert.deepEqual(dedupContext, { reducerLabel: "dedup-0001", claimedWorkerIds: ["worker-001"] diff --git a/plugins/codex-security/references/final-report.md b/plugins/codex-security/references/final-report.md index 8fa17bd1e..8138be244 100644 --- a/plugins/codex-security/references/final-report.md +++ b/plugins/codex-security/references/final-report.md @@ -59,7 +59,7 @@ Group observations only when they share the same broken security control and eff Set the finding category and CWE from the primary broken control. Do not add secondary support-impact CWEs, such as data exposure or missing authentication, to an injection/RCE/path/file/parser finding merely because they make exploitation worse; mention those impacts in prose or emit a separate finding if that secondary control is independently vulnerable. -Workbench-owned Standard scans submit their source-backed final findings and coverage directly through `record_codex_security_scan_draft`; SDK-owned Standard scans write the same semantics into unsealed canonical files. Deep scans semantically aggregate complete Standard worker results, preserving validated findings, attack-path analysis, affected locations, threat-model context, and honest coverage. The coordinator writes the parent scan's canonical draft; the parent does not read candidate ledgers, run separate validation or attack-path phases, or submit another draft. Canonical `severity.changeConditions` must be one non-empty string; when `attack_path.change_conditions` contains multiple strings, join them into one prose string before writing `findings.json`. +Workbench-owned Standard scans submit their source-backed final findings and coverage directly through `record_codex_security_scan_draft`; SDK-owned Standard scans write the same semantics into unsealed canonical files. Deep scans semantically aggregate complete Standard worker results, preserving validated findings, attack-path analysis, affected locations, and threat-model context. Deep reducers exchange findings and optional scope and threat-model context; their inputs and outputs have no coverage field. The coordinator writes the parent scan's canonical draft and derives its compatibility `coverage.json` from the configured scope and execution outcome. Successful Deep aggregates have empty surfaces, explicit exclusions, and deferred-work arrays, with no coverage notes or open questions. Configured include and exclude paths remain in the scope and report. The parent does not read candidate ledgers, run separate validation or attack-path phases, or submit another draft. Canonical `severity.changeConditions` must be one non-empty string; when `attack_path.change_conditions` contains multiple strings, join them into one prose string before writing `findings.json`. For workbench-backed diff candidates, apply row outcomes in this order: validation disposition `reportable` plus attack-path decision `reportable` becomes a finding with its distinct instance and all relevant entrypoint, root-control, sink, and supporting locations; otherwise, a `deferred` result from either phase becomes `needs_follow_up` coverage and a `coverage.deferred` entry using the recorded uncertainty or proof gap; otherwise, validation disposition `not_applicable` becomes `not_applicable` coverage; otherwise, validation disposition `suppressed` or attack-path decision `ignore` becomes `rejected` coverage. A missing required phase record leaves the candidate unresolved and prevents complete coverage. Do not require phase receipts, per-candidate narratives, or another reconciliation pass. diff --git a/plugins/codex-security/references/scan-artifacts.md b/plugins/codex-security/references/scan-artifacts.md index e4fabb441..3e28f0b9c 100644 --- a/plugins/codex-security/references/scan-artifacts.md +++ b/plugins/codex-security/references/scan-artifacts.md @@ -42,6 +42,8 @@ End each repository-scoped threat model with these two lines: Workbench-owned Standard scans submit findings and coverage through `record_codex_security_scan_draft`; SDK-owned Standard scans write unsealed canonical files directly. Deep scans run complete Standard scan workers, each of which saves `complete: false` checkpoints and submits its final validated findings, coverage, threat model, and optional scope through its bound `record_codex_security_scan_draft` tool. Pending candidates retain their original evidence in `coverage.deferred`. Host-owned immutable checkpoints survive retries, cancellation, and failure; a checkpoint alone is never an accepted complete worker result. The coordinator semantically reduces those complete results and writes the parent scan's unsealed `scan-manifest.json`, `findings.json`, and `coverage.json`. The parent does not list candidates, rerun validation or attack-path phases, or submit another draft. Workbench-backed diff scans retain the compact artifacts described below. +Deep reducer inputs, results, and checkpoints contain findings and optional scope and threat-model context, with no coverage field. Worker coverage remains in the original Standard results. The host writes the parent's compatibility `coverage.json` using configured paths and its execution outcome; successful aggregates have empty review-observation arrays. + - A workbench-backed diff scan records all candidates once with `record_codex_security_discovery_candidates({ scanId, candidates })` and reads the canonical candidates with `list_codex_security_candidates({ scanId, cursor?, limit? })`. - The writer validates candidates against assigned source paths, merges rows with the same CWE ids, locations, and optional instance, preserves their text, and assigns deterministic `candidate_id` values. - After normalization, compact validation adds exactly one `validation` object to every row with `disposition` (`reportable`, `suppressed`, `not_applicable`, or `deferred`), `method`, `confidence` (`high`, `medium`, or `low`), `confidence_rationale`, concise `rubric` and `evidence`, `counterevidence_or_proof_gap`, `remaining_uncertainty`, and optional `artifact_paths`. Add `source`, `control`, `sink`, or `preconditions` only when they clarify or differ from the discovery fields. diff --git a/plugins/codex-security/references/scan-contract.md b/plugins/codex-security/references/scan-contract.md index f49de0975..9b05c6283 100644 --- a/plugins/codex-security/references/scan-contract.md +++ b/plugins/codex-security/references/scan-contract.md @@ -106,7 +106,9 @@ Use CWE taxonomy separately. Do not include file names, line numbers, scan IDs, `coverage.json` prevents downstream consumers from confusing `not observed` with `not scanned`. -Record: +Deep parent scans keep this file for compatibility. The host copies the configured include and exclude paths and derives completeness from the coordinator's outcome. An accepted aggregate uses `complete` with empty `surfaces`, `explicitExclusions`, and `deferred` arrays and no `openQuestions`. Deep reducer inputs and outputs contain no coverage. A time limit reached before any review completes retains the host's partial outcome and diagnostic; stopped scans retain their existing recovery behavior. Standard workers keep their own coverage in their saved results. + +For Standard and diff scans, record: - scan mode and inventory strategy - included and excluded paths @@ -140,7 +142,7 @@ For a whole-repository Deep scan, keep `inventoryStrategy` as `repository`; repe | `directory` | Deterministic non-Git directory inventory | | `custom` | Producer-defined inventory described by detailed receipts | -Use `complete` when the requested scope was fully reviewed, `partial` when in-scope work was deferred, and `unknown` when the producer cannot establish enough coverage to make that distinction. +For Standard and diff scans, use `complete` when the requested scope was fully reviewed, `partial` when in-scope work was deferred, and `unknown` when the producer cannot establish enough coverage to make that distinction. Map detailed ledger closure into completed surface summaries in this order: diff --git a/plugins/codex-security/schemas/tools/deep-reducer.schema.json b/plugins/codex-security/schemas/tools/deep-reducer.schema.json index 87dd5896b..69726ccdc 100644 --- a/plugins/codex-security/schemas/tools/deep-reducer.schema.json +++ b/plugins/codex-security/schemas/tools/deep-reducer.schema.json @@ -29,32 +29,11 @@ }, "findings": { "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/scanDraftInput/properties/findings" - }, - "coverage": { - "type": "object", - "description": "Reviewed surfaces, exclusions, and open questions. The host sets coverage completeness.", - "properties": { - "surfaces": { - "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/coverage/properties/surfaces" - }, - "explicitExclusions": { - "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/coverage/properties/explicitExclusions" - }, - "openQuestions": { - "$ref": "codex-security://schemas/tools/scan-draft.schema.json#/$defs/coverage/properties/openQuestions" - } - }, - "required": [ - "surfaces", - "explicitExclusions" - ], - "additionalProperties": true } }, "required": [ "scanId", - "findings", - "coverage" + "findings" ], "additionalProperties": false } diff --git a/plugins/codex-security/scripts/report_projection.py b/plugins/codex-security/scripts/report_projection.py index 697dda8bd..98b4f00d1 100644 --- a/plugins/codex-security/scripts/report_projection.py +++ b/plugins/codex-security/scripts/report_projection.py @@ -814,7 +814,7 @@ def build_report_markdown( "", _text( scope.get("summary"), - "The scan reviewed the canonical include paths and exclusions listed below.", + "The scan was configured for the include paths and exclusions listed below.", ), "", f"- Scan mode: {coverage['mode']}", diff --git a/plugins/codex-security/scripts/workbench_saved_results.py b/plugins/codex-security/scripts/workbench_saved_results.py index 56288ee4c..9f77ac170 100644 --- a/plugins/codex-security/scripts/workbench_saved_results.py +++ b/plugins/codex-security/scripts/workbench_saved_results.py @@ -155,8 +155,10 @@ def _read_saved_result(scan_dir: Path, relative: str, scan_id: str) -> tuple[dic draft = _read_scan_local_json(scan_dir, relative, "Saved scan checkpoint") if draft.get("scanId") != scan_id: raise ContractError("checkpoint belongs to a different scan") - if not isinstance(draft.get("findings"), list) or not isinstance(draft.get("coverage"), dict): - raise ContractError("checkpoint has no semantic findings or coverage") + if not isinstance(draft.get("findings"), list) or ( + "coverage" in draft and not isinstance(draft["coverage"], dict) + ): + raise ContractError("checkpoint has no semantic findings or has invalid coverage") return draft, _digest(draft) @@ -574,7 +576,9 @@ def reducer_output(directory: str, attempt: int, reducer_worker: Any) -> None: if frozen_source_digests is not None and frozen_source_digests[relative] != digest: raise ContractError("checkpoint changed after the scan stopped") source_digests[relative] = digest - sources.append((relative, draft, worker_id)) + # Deep reducers save findings without coverage. Keep their original + # digest while supplying empty observations to the recovery union. + sources.append((relative, {"coverage": {}, **draft}, worker_id)) except (ContractError, OSError, ValueError) as exc: if (scan_dir / relative).exists(): warnings.append(f"Preserved unreadable checkpoint {relative}: {exc}") @@ -645,7 +649,7 @@ def reducer_output(directory: str, attempt: int, reducer_worker: Any) -> None: manifest["scan"]["preservedSources"] = source_digests coverage = ( copy.deepcopy(parent["coverage"]) - if parent + if parent and parent["coverage"] else { "completeness": "partial", "mode": binding["coverageMode"], diff --git a/plugins/codex-security/skills/deep-security-scan/SKILL.md b/plugins/codex-security/skills/deep-security-scan/SKILL.md index 72007f5c8..61707fa40 100644 --- a/plugins/codex-security/skills/deep-security-scan/SKILL.md +++ b/plugins/codex-security/skills/deep-security-scan/SKILL.md @@ -7,6 +7,8 @@ description: Use when the user asks for a deep, exhaustive, multi-pass, or varia Use `start_codex_security_deep_scan` to run repeated independent workers against the exact requested target and scope. Each worker loads `../../references/core-scan.md` directly and performs the complete ordinary Standard audit, including its own threat map, investigation, source-backed validation, and attack-path reasoning, saving worker-bound checkpoints as results arrive and one final semantic scan draft when its audit finishes. The coordinator aggregates the finished Standard results and writes the parent scan's unsealed `scan-manifest.json`, `findings.json`, and `coverage.json` before returning `{ manifestPath }`. +The configured directories and exclusions remain in the scan scope and report. Reducers aggregate findings without coverage inputs or outputs. The coordinator supplies the parent `coverage.json` for compatibility from the configured scope and execution outcome; successful aggregates contain no surface observations, deferred work, or coverage notes. Each Standard worker retains its own coverage in its original result. + ## Phase Ownership The coordinator owns the independent complete Standard scans, aggregation, and canonical parent artifact construction. This thread owns setup, user context, and exactly one final `complete_codex_security_scan` call. Do not rerun worker phases, list candidates, aggregate findings, submit another semantic draft, or start another scan. The returned `manifestPath` identifies the already-authored canonical parent `scan-manifest.json`; completion seals it and generates the report. diff --git a/plugins/codex-security/tests/test_deep_scan_successful_publication.py b/plugins/codex-security/tests/test_deep_scan_successful_publication.py index da924d6b8..84e89168f 100644 --- a/plugins/codex-security/tests/test_deep_scan_successful_publication.py +++ b/plugins/codex-security/tests/test_deep_scan_successful_publication.py @@ -98,7 +98,10 @@ def create(*, mode="deep", scope="."): manifest = json.loads((scan_dir / "scan-manifest.json").read_text()) findings = json.loads((scan_dir / "findings.json").read_text())["findings"] coverage = json.loads((scan_dir / "coverage.json").read_text()) - coverage["openQuestions"] = [{"question": "Which deployment controls apply?"}] + if mode == "deep": + coverage.update(surfaces=[], explicitExclusions=[], deferred=[]) + else: + coverage["openQuestions"] = [{"question": "Which deployment controls apply?"}] for field in ("documentType", "schemaVersion", "scanId"): coverage.pop(field) # Match the ordinary semantic envelopes written by the host draft writer. @@ -172,15 +175,25 @@ def assert_published_aggregate(scan): @pytest.mark.parametrize("scope", [".", "subdir"], ids=["repository", "scoped"]) -def test_deep_publication_uses_aggregate_without_worker_coverage( +def test_deep_publication_keeps_configured_scope_without_worker_observations( workbench_api, workbench_db, publication_scan, scope ): scan = publication_scan(scope=scope) result = add_worker(workbench_db, scan) - worker_coverage = copy.deepcopy(scan.coverage) - worker_coverage["completeness"] = "partial" - worker_coverage["surfaces"][0]["disposition"] = "needs_follow_up" - worker_coverage["deferred"] = [{"id": "worker-follow-up", "reason": "Review this path again."}] + worker_coverage = { + "completeness": "partial", + "surfaces": [ + { + "id": "worker-surface", + "label": "Worker review", + "disposition": "needs_follow_up", + "receiptRefs": [], + } + ], + "explicitExclusions": [{"pattern": "docs/", "reason": "Worker-local exclusion."}], + "deferred": [{"id": "worker-follow-up", "reason": "Review this path again."}], + "openQuestions": [{"question": "Which deployment controls apply?"}], + } result.write_text( json.dumps( { @@ -198,6 +211,15 @@ def test_deep_publication_uses_aggregate_without_worker_coverage( assert completed["progress"]["status"] == "complete" assert_published_aggregate(scan) assert result.read_bytes() == source_bytes + coverage = json.loads((scan.scan_dir / "coverage.json").read_text()) + assert coverage["mode"] == ("deep_repository" if scope == "." else "scoped_path") + assert coverage["includePaths"] == [scope] + assert coverage["excludePaths"] == [] + report = (scan.scan_dir / "report.md").read_text() + assert f"- Included paths: {scope}" in report + assert "- Excluded paths: none" in report + assert "## Reviewed Surfaces" not in report + assert "Which deployment controls apply?" not in report def test_deep_publication_ignores_empty_canceled_checkpoint( @@ -266,35 +288,65 @@ def test_deep_prepare_and_complete_preserve_the_same_aggregate( assert_published_aggregate(scan) -def test_stopped_deep_scan_still_salvages_checkpoint_findings( - workbench_api, workbench_db, publication_scan +@pytest.mark.parametrize( + ("source", "scope", "has_parent"), + [ + ("standard-worker-checkpoint", ".", True), + ("deep-reducer-checkpoint", ".", True), + ("deep-reducer-result", ".", True), + ("deep-reducer-result", "subdir", False), + ], + ids=[ + "standard-worker-checkpoint", + "deep-reducer-checkpoint", + "deep-reducer-result", + "scoped-reducer-without-parent", + ], +) +def test_stopped_deep_scan_still_salvages_saved_findings( + workbench_api, workbench_db, publication_scan, source, scope, has_parent ): - scan = publication_scan() + scan = publication_scan(scope=scope) + if not has_parent: + for name in ("scan-manifest.json", "findings.json", "coverage.json"): + (scan.scan_dir / name).unlink() with workbench_db: workbench_db.execute( "UPDATE deep_scan_runs SET status = 'running', phase = 'reducing', " "terminal_reason = NULL, completed_at = NULL WHERE scan_id = ?", (scan.scan_id,), ) - result = add_worker(workbench_db, scan, status="running") + result = add_worker( + workbench_db, scan, status="succeeded" if source == "deep-reducer-result" else "running" + ) + if source.startswith("deep-reducer"): + with workbench_db: + workbench_db.execute( + "UPDATE deep_scan_workers SET kind = 'dedup', merge_state = 'none' WHERE id = ?", + (result.parent.name,), + ) later_finding = copy.deepcopy(scan.findings[0]) later_finding["identity"]["anchor"] = "later-checkpoint-finding" later_finding["summary"] = "Finding saved after the last completed aggregate." - checkpoint = write_checkpoint( - result.parent / "checkpoints", - { - "scanId": scan.scan_id, - "complete": False, - "findings": [later_finding], - "coverage": { - "completeness": "partial", - "surfaces": [], - "explicitExclusions": [], - "deferred": [], - }, - }, - ) - result.write_text("{interrupted worker output") + saved = { + "scanId": scan.scan_id, + "complete": source == "deep-reducer-result", + "findings": [later_finding], + } + if source == "standard-worker-checkpoint": + saved["coverage"] = { + "completeness": "partial", + "surfaces": [], + "explicitExclusions": [], + "deferred": [], + } + if source == "deep-reducer-result": + checkpoint = None + result.write_text(json.dumps(saved)) + else: + checkpoint = write_checkpoint(result.parent / "checkpoints", saved) + result.write_text("{interrupted worker output") + result_bytes = result.read_bytes() stopped = workbench_api["fail_scan"]( workbench_db, @@ -309,12 +361,21 @@ def test_stopped_deep_scan_still_salvages_checkpoint_findings( assert manifest["scan"]["status"] == "failed" assert coverage["completeness"] == "partial" findings = json.loads((scan.scan_dir / "findings.json").read_text())["findings"] - assert {finding["summary"] for finding in findings} == { - scan.findings[0]["summary"], - later_finding["summary"], - } - assert checkpoint.is_file() - assert result.read_text() == "{interrupted worker output" + expected_summaries = {later_finding["summary"]} + if has_parent: + expected_summaries.add(scan.findings[0]["summary"]) + assert {finding["summary"] for finding in findings} == expected_summaries + + artifact_names = ("scan-manifest.json", "findings.json", "coverage.json") + published = {name: (scan.scan_dir / name).read_bytes() for name in artifact_names} + recovered = workbench_api["recover_scan_results"]( + workbench_db, Namespace(scan_id=scan.scan_id) + )["scan"] + assert recovered["findingCount"] == len(expected_summaries) + assert {name: (scan.scan_dir / name).read_bytes() for name in artifact_names} == published + if checkpoint is not None: + assert json.loads(checkpoint.read_text()) == saved + assert result.read_bytes() == result_bytes def test_standard_publication_preserves_deliberately_partial_coverage( diff --git a/plugins/codex-security/tests/test_scan_contract_examples.py b/plugins/codex-security/tests/test_scan_contract_examples.py index f16fbce4f..f3e27451b 100644 --- a/plugins/codex-security/tests/test_scan_contract_examples.py +++ b/plugins/codex-security/tests/test_scan_contract_examples.py @@ -54,15 +54,14 @@ def test_schemas_are_valid_draft_2020_12(self) -> None: with self.subTest(schema=schema_path.name): Draft202012Validator.check_schema(read_json(schema_path)) - def test_deep_reducer_schema_accepts_factual_coverage_and_standard_findings(self) -> None: + def test_deep_reducer_schema_accepts_standard_findings_without_coverage(self) -> None: common_schema = read_json(SCHEMA_DIR / "definitions" / "artifact-common.schema.json") scan_draft_schema = read_json(SCHEMA_DIR / "tools" / "scan-draft.schema.json") reducer_schema = read_json(SCHEMA_DIR / "tools" / "deep-reducer.schema.json") - reducer_coverage = reducer_schema["$defs"]["reductionInput"]["properties"]["coverage"] - for field in ("completeness", "deferred"): - self.assertNotIn(field, reducer_coverage["properties"]) - self.assertNotIn(field, reducer_coverage["required"]) - self.assertEqual(set(reducer_coverage["required"]), {"surfaces", "explicitExclusions"}) + reduction_input = reducer_schema["$defs"]["reductionInput"] + self.assertNotIn("coverage", reduction_input["properties"]) + self.assertEqual(set(reduction_input["required"]), {"scanId", "findings"}) + self.assertFalse(reduction_input["additionalProperties"]) registry = Registry().with_resources( (schema["$id"], Resource.from_contents(schema)) for schema in (common_schema, scan_draft_schema) @@ -96,39 +95,20 @@ def test_deep_reducer_schema_accepts_factual_coverage_and_standard_findings(self request = { "scanId": "7fc17317-9594-49e0-b06a-d72fd7e14bba", "findings": [finding], - "coverage": coverage, } validator.validate(request) validator.validate( { **request, + "complete": True, + "handoffClaimToken": "2ea75b4f-f9b2-49b4-a5a9-2a8de8ca9047", "scope": {"summary": "HTTP responses"}, "threatModel": {"summary": "Untrusted requests reach responses."}, } ) validator.validate({**request, "findings": []}) - validator.validate( - { - **request, - "coverage": { - **coverage, - "openQuestions": [{"question": "Which deployments expose this route?"}], - }, - } - ) - for completeness in ("complete", "partial", "unknown"): - with self.subTest(legacy_completeness=completeness): - validator.validate( - { - **request, - "coverage": { - **coverage, - "completeness": completeness, - "deferred": [{"id": "old-follow-up", "reason": "Legacy review note."}], - }, - } - ) + self.assertFalse(validator.is_valid({**request, "coverage": coverage})) standard_validator = Draft202012Validator( scan_draft_schema, registry=registry, format_checker=FormatChecker() @@ -139,7 +119,27 @@ def test_deep_reducer_schema_accepts_factual_coverage_and_standard_findings(self } standard_validator.validate(standard_request) self.assertFalse(standard_validator.is_valid(request)) - for missing_field in ("completeness", "deferred"): + self.assertFalse(standard_validator.is_valid({**request, "coverage": coverage})) + for completeness in ("complete", "partial", "unknown"): + with self.subTest(standard_completeness=completeness): + standard_coverage_request = { + **standard_request, + "coverage": { + **standard_request["coverage"], + "completeness": completeness, + }, + } + standard_validator.validate(standard_coverage_request) + self.assertFalse(validator.is_valid(standard_coverage_request)) + self.assertFalse( + standard_validator.is_valid( + { + **standard_request, + "coverage": {**standard_request["coverage"], "completeness": "invalid"}, + } + ) + ) + for missing_field in ("completeness", "surfaces", "explicitExclusions", "deferred"): with self.subTest(standard_missing_coverage_field=missing_field): self.assertFalse( standard_validator.is_valid( @@ -153,21 +153,6 @@ def test_deep_reducer_schema_accepts_factual_coverage_and_standard_findings(self } ) ) - for missing_field in ("surfaces", "explicitExclusions"): - with self.subTest(deep_missing_coverage_field=missing_field): - self.assertFalse( - validator.is_valid( - { - **request, - "coverage": { - field: value - for field, value in coverage.items() - if field != missing_field - }, - } - ) - ) - for extra_field in ( "source_worker_id", "unknown_field", @@ -200,7 +185,7 @@ def test_deep_reducer_schema_accepts_factual_coverage_and_standard_findings(self ) ) - for missing_field in ("scanId", "findings", "coverage"): + for missing_field in ("scanId", "findings"): with self.subTest(missing_field=missing_field): self.assertFalse( validator.is_valid( diff --git a/sdk/typescript/tests-ts/deep-scan-reducer-recovery.test.ts b/sdk/typescript/tests-ts/deep-scan-reducer-recovery.test.ts index c33104c52..b3905c4ab 100644 --- a/sdk/typescript/tests-ts/deep-scan-reducer-recovery.test.ts +++ b/sdk/typescript/tests-ts/deep-scan-reducer-recovery.test.ts @@ -14,7 +14,7 @@ function bundledFunction(runtime: string, name: string): string { return source; } -test("keeps every advertised Deep worker tool within Codex's name limit", async () => { +test("advertises distinct Standard worker and Deep reducer contracts", async () => { const runtime = await loadBundledRuntime(); const method = / compactArtifactServer\(request\) \{[\s\S]*?\n \}/u.exec( runtime, @@ -83,7 +83,12 @@ test("keeps every advertised Deep worker tool within Codex's name limit", async .split("\n") .map((line) => JSON.parse(line) as { id?: number; result?: unknown }) .find((message) => message.id === 2)?.result as - | { tools: Array<{ name: string }> } + | { + tools: Array<{ + name: string; + inputSchema: { properties: Record }; + }>; + } | undefined; expect(response).toBeDefined(); expect(response!.tools.length).toBeGreaterThan(0); @@ -92,11 +97,22 @@ test("keeps every advertised Deep worker tool within Codex's name limit", async 64, ); } - if (layout === "reducer") { - expect(response!.tools.map((tool) => tool.name)).toContain( - "record_codex_security_deep_reduction", + const recordTool = response!.tools.find( + (tool) => + tool.name === + (layout === "reducer" + ? "record_codex_security_deep_reduction" + : "record_codex_security_scan_draft"), + ); + expect(recordTool).toBeDefined(); + expect(recordTool!.inputSchema.properties).toHaveProperty("findings"); + expect(recordTool!.inputSchema.properties).toHaveProperty("scope"); + if (layout === "reducer") + expect(recordTool!.inputSchema.properties).not.toHaveProperty( + "coverage", ); - } + else + expect(recordTool!.inputSchema.properties).toHaveProperty("coverage"); } } finally { rmSync(root, { recursive: true, force: true }); From dc9c6376e0916a8496e152a6198c852ad5b6c7aa Mon Sep 17 00:00:00 2001 From: Dane Schneider <269480009+daneschneider-oai@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:09:21 -0700 Subject: [PATCH 08/10] docs(deep-scan): clarify comments and workflow guidance --- .../codex-security/mcp-app/src/artifact-deep-reducer.ts | 4 ++-- plugins/codex-security/mcp-app/src/artifact-scan-draft.ts | 4 ++-- .../mcp-app/src/deep-scan/artifact-validation.ts | 5 ++++- .../codex-security/mcp-app/src/deep-scan/coordinator.ts | 7 +++---- .../mcp-app/tests/deep_scan_publication_cases.mjs | 2 +- plugins/codex-security/references/final-report.md | 8 +++++--- plugins/codex-security/references/scan-artifacts.md | 6 ++++-- plugins/codex-security/references/scan-contract.md | 6 ++++-- plugins/codex-security/scripts/deep_scan_workbench.py | 4 ++-- plugins/codex-security/scripts/finalize_scan_contract.py | 2 +- plugins/codex-security/scripts/workbench_db.py | 4 ++-- plugins/codex-security/scripts/workbench_saved_results.py | 4 ++-- plugins/codex-security/skills/deep-security-scan/SKILL.md | 4 ++-- .../tests/test_deep_scan_successful_publication.py | 5 ++--- 14 files changed, 36 insertions(+), 29 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts b/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts index f5d9f2f14..a2400d002 100644 --- a/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts +++ b/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts @@ -58,7 +58,7 @@ interface BoundReducer { scanId?: string; } -/** Return assigned findings and context without exposing worker artifact locations. */ +/** Read the findings and scan context assigned to this reducer. */ export async function getCodexSecurityDeepReducerInputs( context: ArtifactContext ): Promise { @@ -101,7 +101,7 @@ export async function getCodexSecurityDeepReducerInputs( }); } -/** Validate and durably replace this reducer's complete semantic result. */ +/** Check and save the reducer's finished result. */ export async function recordCodexSecurityDeepReduction( context: ArtifactContext, input: unknown diff --git a/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts b/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts index 65a879f14..4c560822c 100644 --- a/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts +++ b/plugins/codex-security/mcp-app/src/artifact-scan-draft.ts @@ -88,8 +88,8 @@ export async function recordCodexSecurityScanDraft( for (;;) { signal?.throwIfAborted(); - // The coordinator has already accepted and reconciled a terminal Deep - // aggregate. Publishing it must not reopen checkpoints or prior drafts. + // Deep results are ready to save. Do not merge older drafts or + // checkpoints into them. const preserved = context.mode === "deep" && parsed.complete !== false ? { input: parsed, previousDigest: undefined } : await preserveScanDraft(context, parsed, false); diff --git a/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts b/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts index e3d0ac4e5..9f884ce2e 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts @@ -22,7 +22,10 @@ export interface ReducerArtifactValidation { result: DeepReductionInput; } -/** Reuse Standard finding validation without admitting legacy reducer coverage. */ +/** + * Check reducer findings with the Standard scan validator. + * It requires coverage, so add an empty value and remove it after validation. + */ export function parseDeepReduction( input: Record, persisted = false, diff --git a/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts b/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts index 83e60dd76..905c0c9a8 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts @@ -295,8 +295,8 @@ export class DeepScanCoordinator { const draft = schedulerResult.result ? { ...structuredClone(schedulerResult.result), - // Keep the saved coverage contract for existing readers. Deep - // completion comes from the coordinator, not worker observations. + // Readers require coverage.json. The coordinator has accepted this + // result, so mark it complete and leave review notes empty. coverage: { completeness: "complete", surfaces: [], @@ -949,8 +949,7 @@ export class DeepScanCoordinator { this.audit.canceledWorkerIds = unique(canceledWorkerIds); this.audit.bufferedWorkerIds = buffer.map((worker) => worker.id); - // Saturation fixes the aggregate at the stop boundary. Failures from workers - // still settling after cancellation cannot overturn that completed result. + // Once Deep reaches saturation, late worker errors cannot fail the scan. if (lateFailure && stopReason !== "saturated") throw lateFailure; if ( diff --git a/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs b/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs index 11b62f5a8..5e44d1796 100644 --- a/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs +++ b/plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs @@ -19,7 +19,7 @@ export async function testDeepScanPublication({ if (update.kind === "discovery" && update.status === "succeeded" && path.basename(path.dirname(update.promptPath)) === "discovery-0003") { acceptedLateWorker = persisted; - // A worker still settling when saturation is reached is omitted. + // This worker finishes too late to be included in the final result. await rm(update.resultManifestPath); lateAcceptance.resolve(); await releaseAcceptance.promise; diff --git a/plugins/codex-security/references/final-report.md b/plugins/codex-security/references/final-report.md index 8138be244..bf7c6eefa 100644 --- a/plugins/codex-security/references/final-report.md +++ b/plugins/codex-security/references/final-report.md @@ -16,11 +16,11 @@ Use `report.md` as the primary readable entry point. Explain report-relevant art In the final response, link the generated markdown report path as the primary readable artifact. -Every scan mode uses the same final report pipeline. Workbench-owned Standard and workbench-backed diff scans submit canonical semantics with `record_codex_security_scan_draft({ scanId, handoffClaimToken?, scope?, threatModel?, findings, coverage })`; the workbench supplies authoritative metadata and writes the unsealed canonical draft. Deep Standard scan workers submit complete semantic results through their bound draft tool, and the coordinator aggregates them and writes the parent scan's unsealed canonical draft. SDK-owned Standard scans instead write unsealed canonical files with the exact SDK-provided metadata and leave finalization to the SDK. Terminal scans without a `scanId` retain their existing canonical JSON workflow. No mode authors, repairs, or treats an existing `report.md` as input. Finalization validates and enriches the canonical JSON, seals the canonical JSON and evidence artifacts, then deterministically generates `report.md`. Supply report prose through structured canonical semantics rather than a separately authored report. +Every scan mode uses the same final report pipeline. Workbench-owned Standard and workbench-backed diff scans submit canonical semantics with `record_codex_security_scan_draft({ scanId, handoffClaimToken?, scope?, threatModel?, findings, coverage })`; the workbench supplies authoritative metadata and writes the unsealed canonical draft. For Deep scans, the coordinator writes the parent scan's unsealed canonical draft from its accepted aggregate. SDK-owned Standard scans instead write unsealed canonical files with the exact SDK-provided metadata and leave finalization to the SDK. Terminal scans without a `scanId` retain their existing canonical JSON workflow. No mode authors, repairs, or treats an existing `report.md` as input. Finalization validates and enriches the canonical JSON, seals the canonical JSON and evidence artifacts, then deterministically generates `report.md`. Supply report prose through structured canonical semantics rather than a separately authored report. For each finding, supply an evidence-supported lowercase vulnerability-family `ruleId`; `taxonomy: { category, cwe }` using its exact known CWEs; verified locations; and `provenance.source`, using `"local_plugin"` only when this plugin actually discovered the finding. Preserve genuine worker or source provenance and any existing canonical candidate identity in the finding extensions. A finding with no known CWE retains `cwe: []`; never invent a classification. Include optional `codeEvidence` only when its actual code is nonempty and every referenced evidence ID is present. -Supply semantic coverage as `{ completeness, surfaces, explicitExclusions, deferred }`, with each surface using the actual `label` and one existing `disposition`. Mark coverage `partial` when a deferred item or `needs_follow_up` surface remains; preserve its real reason and supporting context. Each deferred item needs a meaningful reason; preserve any existing `id` or `candidateId`. The workbench derives a missing ID from its candidate identity or stable deferred-work details. Open questions may be nonempty strings or `{ question, followUpPrompt? }` objects. The workbench derives target and scope metadata, scope include and exclude paths, coverage mode and inventory strategy, finding identities and fingerprints, and surface IDs. Do not put those workbench-owned values or top-level coverage receipt references into the semantic draft. +For Standard scan drafts, including Deep worker results, and diff drafts, supply semantic coverage as `{ completeness, surfaces, explicitExclusions, deferred }`, with each surface using the actual `label` and one existing `disposition`. Mark coverage `partial` when a deferred item or `needs_follow_up` surface remains; preserve its real reason and supporting context. Each deferred item needs a meaningful reason; preserve any existing `id` or `candidateId`. The workbench derives a missing ID from its candidate identity or stable deferred-work details. Open questions may be nonempty strings or `{ question, followUpPrompt? }` objects. The workbench derives target and scope metadata, scope include and exclude paths, coverage mode and inventory strategy, finding identities and fingerprints, and surface IDs. Do not put those workbench-owned values or top-level coverage receipt references into the semantic draft. During a host-backed scan, checkpoint saved findings and pending candidates with `complete: false`; a checkpoint is not a completed audit. After a final workbench-owned Standard or workbench-backed diff draft is accepted with `complete: true` (or the backwards-compatible omitted flag), or the Deep coordinator returns its parent scan's canonical manifest, call `complete_codex_security_scan({ scanId, handoffClaimToken? })` and use its returned completion metadata. An SDK-owned scan returns its unsealed canonical files without calling a completion tool or finalizer; the SDK owns completion and report generation. Read full canonical results only when explicitly requested. For a terminal/chat workflow without a `scanId` or completion tool, retain `python /scripts/finalize_scan_contract.py --scan-dir --source-root ` after writing the canonical JSON. Outside the SDK path, do not mark the scan goal complete until finalization succeeds and the generated report exists. @@ -59,7 +59,7 @@ Group observations only when they share the same broken security control and eff Set the finding category and CWE from the primary broken control. Do not add secondary support-impact CWEs, such as data exposure or missing authentication, to an injection/RCE/path/file/parser finding merely because they make exploitation worse; mention those impacts in prose or emit a separate finding if that secondary control is independently vulnerable. -Workbench-owned Standard scans submit their source-backed final findings and coverage directly through `record_codex_security_scan_draft`; SDK-owned Standard scans write the same semantics into unsealed canonical files. Deep scans semantically aggregate complete Standard worker results, preserving validated findings, attack-path analysis, affected locations, and threat-model context. Deep reducers exchange findings and optional scope and threat-model context; their inputs and outputs have no coverage field. The coordinator writes the parent scan's canonical draft and derives its compatibility `coverage.json` from the configured scope and execution outcome. Successful Deep aggregates have empty surfaces, explicit exclusions, and deferred-work arrays, with no coverage notes or open questions. Configured include and exclude paths remain in the scope and report. The parent does not read candidate ledgers, run separate validation or attack-path phases, or submit another draft. Canonical `severity.changeConditions` must be one non-empty string; when `attack_path.change_conditions` contains multiple strings, join them into one prose string before writing `findings.json`. +Canonical `severity.changeConditions` must be one non-empty string; when `attack_path.change_conditions` contains multiple strings, join them into one prose string before writing `findings.json`. For workbench-backed diff candidates, apply row outcomes in this order: validation disposition `reportable` plus attack-path decision `reportable` becomes a finding with its distinct instance and all relevant entrypoint, root-control, sink, and supporting locations; otherwise, a `deferred` result from either phase becomes `needs_follow_up` coverage and a `coverage.deferred` entry using the recorded uncertainty or proof gap; otherwise, validation disposition `not_applicable` becomes `not_applicable` coverage; otherwise, validation disposition `suppressed` or attack-path decision `ignore` becomes `rejected` coverage. A missing required phase record leaves the candidate unresolved and prevents complete coverage. Do not require phase receipts, per-candidate narratives, or another reconciliation pass. @@ -73,6 +73,8 @@ Use this report structure: `## Scope` +Deep reports present the configured include and exclude paths and execution outcome alongside aggregated findings, with their validation, attack paths, affected locations, and any supplied scope and threat-model context. When the discovery time limit expires before any source review completes, the report explains the incomplete review. + Populate `scan.scope` with in-scope context, artifacts reviewed, runtime or test status, validation mode, and explicit limitations. Include/exclude paths and coverage fields supply the remaining projected scope content. If the threat model was generated during Phase 1 rather than provided by the user, say that in canonical scope context. Do not call generated threat-model material an external input. After the scope bullets, include a compact `### Scan Summary` table when the scan has findings or repository-wide coverage. Use columns `Field` and `Value`. Include the count of reportable findings, severity mix, confidence mix, coverage, and validation mode when those values are known. Keep artifact paths below this table. diff --git a/plugins/codex-security/references/scan-artifacts.md b/plugins/codex-security/references/scan-artifacts.md index 3e28f0b9c..9d58246d9 100644 --- a/plugins/codex-security/references/scan-artifacts.md +++ b/plugins/codex-security/references/scan-artifacts.md @@ -40,9 +40,11 @@ End each repository-scoped threat model with these two lines: ### Compact Deep And Workbench-Backed Diff Discovery -Workbench-owned Standard scans submit findings and coverage through `record_codex_security_scan_draft`; SDK-owned Standard scans write unsealed canonical files directly. Deep scans run complete Standard scan workers, each of which saves `complete: false` checkpoints and submits its final validated findings, coverage, threat model, and optional scope through its bound `record_codex_security_scan_draft` tool. Pending candidates retain their original evidence in `coverage.deferred`. Host-owned immutable checkpoints survive retries, cancellation, and failure; a checkpoint alone is never an accepted complete worker result. The coordinator semantically reduces those complete results and writes the parent scan's unsealed `scan-manifest.json`, `findings.json`, and `coverage.json`. The parent does not list candidates, rerun validation or attack-path phases, or submit another draft. Workbench-backed diff scans retain the compact artifacts described below. +Workbench-owned Standard scans submit findings and coverage through `record_codex_security_scan_draft`; SDK-owned Standard scans write unsealed canonical files directly. Workbench-backed diff scans use the compact artifacts described below. -Deep reducer inputs, results, and checkpoints contain findings and optional scope and threat-model context, with no coverage field. Worker coverage remains in the original Standard results. The host writes the parent's compatibility `coverage.json` using configured paths and its execution outcome; successful aggregates have empty review-observation arrays. +Deep scans run ordinary Standard scan workers. Workers save progress with `complete: false` and submit final results through `record_codex_security_scan_draft`. Their checkpoints and results contain findings and coverage, with optional scope and threat-model context. Pending candidates and their evidence are recorded in worker coverage. Immutable checkpoints store progress across retries, cancellation, and failure. The coordinator passes completed workers' findings and context to the reducer. + +Deep reducer inputs, results, and checkpoints contain findings and optional scope and threat-model context. The host writes the parent scan's unsealed `scan-manifest.json` and `findings.json` from the accepted aggregate, and derives `coverage.json` from the configured include and exclude paths and the coordinator's outcome. The parent completes the scan from these artifacts. If the discovery time limit expires before any source review completes, the parent records partial coverage with that reason. See `scan-contract.md` for canonical field definitions. - A workbench-backed diff scan records all candidates once with `record_codex_security_discovery_candidates({ scanId, candidates })` and reads the canonical candidates with `list_codex_security_candidates({ scanId, cursor?, limit? })`. - The writer validates candidates against assigned source paths, merges rows with the same CWE ids, locations, and optional instance, preserves their text, and assigns deterministic `candidate_id` values. diff --git a/plugins/codex-security/references/scan-contract.md b/plugins/codex-security/references/scan-contract.md index 9b05c6283..d5f0b105e 100644 --- a/plugins/codex-security/references/scan-contract.md +++ b/plugins/codex-security/references/scan-contract.md @@ -104,9 +104,11 @@ Use CWE taxonomy separately. Do not include file names, line numbers, scan IDs, ## Coverage -`coverage.json` prevents downstream consumers from confusing `not observed` with `not scanned`. +`coverage.json` records scan scope and completion information. Standard and diff summaries also describe reviewed surfaces and outstanding work. -Deep parent scans keep this file for compatibility. The host copies the configured include and exclude paths and derives completeness from the coordinator's outcome. An accepted aggregate uses `complete` with empty `surfaces`, `explicitExclusions`, and `deferred` arrays and no `openQuestions`. Deep reducer inputs and outputs contain no coverage. A time limit reached before any review completes retains the host's partial outcome and diagnostic; stopped scans retain their existing recovery behavior. Standard workers keep their own coverage in their saved results. +For a Deep parent scan, the host copies the configured paths into `includePaths` and `excludePaths` and sets `completeness` from the coordinator's outcome. A successful aggregate uses `complete`; `surfaces`, `explicitExclusions`, and `deferred` are empty arrays, and `openQuestions` is omitted. If the configured time limit expires before any review completes, the coordinator writes `partial` and records the explanation in `deferred`. Stopped outcomes follow the [stopped-result recovery rules](#stopped-result-recovery). + +Each Deep worker writes an ordinary Standard result, including its own coverage. A reducer submits `record_codex_security_deep_reduction({ scanId, findings, scope?, threatModel? })`; its saved results and checkpoints contain the accepted findings and optional scope and threat-model context. For Standard and diff scans, record: diff --git a/plugins/codex-security/scripts/deep_scan_workbench.py b/plugins/codex-security/scripts/deep_scan_workbench.py index 799ccea43..853094a0a 100644 --- a/plugins/codex-security/scripts/deep_scan_workbench.py +++ b/plugins/codex-security/scripts/deep_scan_workbench.py @@ -1908,8 +1908,8 @@ def finish_deep_scan_locked( if failed_worker is not None and not failure_capped: raise SystemExit("Deep Scan cannot finish after a worker has failed.") if args.terminal_reason == "saturated": - # The coordinator has stopped discovery. Its remaining workers cannot - # override that outcome if their own cancellation writes failed. + # Mark any remaining workers canceled, including those whose own + # cancellation writes failed, so they cannot block completion. cancel_active_workers(connection, scan_id, now()) active_worker = connection.execute( """ diff --git a/plugins/codex-security/scripts/finalize_scan_contract.py b/plugins/codex-security/scripts/finalize_scan_contract.py index ea9982a6d..58cac8f1f 100644 --- a/plugins/codex-security/scripts/finalize_scan_contract.py +++ b/plugins/codex-security/scripts/finalize_scan_contract.py @@ -1252,7 +1252,7 @@ def _normalize_unsealed_deep_repository_inventory_strategy( *, expected_coverage_mode: str | None, ) -> None: - """Derive the inventory label from the selected Deep repository mode.""" + """Label whole-repository Deep scans as using the repository inventory.""" if expected_coverage_mode == "deep_repository": coverage["inventoryStrategy"] = "repository" diff --git a/plugins/codex-security/scripts/workbench_db.py b/plugins/codex-security/scripts/workbench_db.py index 7e04b1dda..702fcd6bc 100644 --- a/plugins/codex-security/scripts/workbench_db.py +++ b/plugins/codex-security/scripts/workbench_db.py @@ -1534,8 +1534,8 @@ def add_warning() -> None: scan_dir, expected_coverage_mode=expected_coverage_mode(scan), completion_binding=completion_binding, - # Deep has already accepted its aggregate. Publication must not - # recover worker-local drafts or revise the aggregate's coverage. + # Save the finished Deep result as submitted. Worker drafts and + # recovery repairs belong to the stopped-scan path. completion_warnings=warnings if scan["mode"] != "deep" else None, draft_documents=saved_results.merge_saved_results( scan_dir, diff --git a/plugins/codex-security/scripts/workbench_saved_results.py b/plugins/codex-security/scripts/workbench_saved_results.py index 9f77ac170..c20774487 100644 --- a/plugins/codex-security/scripts/workbench_saved_results.py +++ b/plugins/codex-security/scripts/workbench_saved_results.py @@ -576,8 +576,8 @@ def reducer_output(directory: str, attempt: int, reducer_worker: Any) -> None: if frozen_source_digests is not None and frozen_source_digests[relative] != digest: raise ContractError("checkpoint changed after the scan stopped") source_digests[relative] = digest - # Deep reducers save findings without coverage. Keep their original - # digest while supplying empty observations to the recovery union. + # Recovery expects coverage, but reducer results only contain findings + # and context. Add an empty value after hashing the original result. sources.append((relative, {"coverage": {}, **draft}, worker_id)) except (ContractError, OSError, ValueError) as exc: if (scan_dir / relative).exists(): diff --git a/plugins/codex-security/skills/deep-security-scan/SKILL.md b/plugins/codex-security/skills/deep-security-scan/SKILL.md index 61707fa40..ef9b63035 100644 --- a/plugins/codex-security/skills/deep-security-scan/SKILL.md +++ b/plugins/codex-security/skills/deep-security-scan/SKILL.md @@ -5,9 +5,9 @@ description: Use when the user asks for a deep, exhaustive, multi-pass, or varia # Deep Security Scan -Use `start_codex_security_deep_scan` to run repeated independent workers against the exact requested target and scope. Each worker loads `../../references/core-scan.md` directly and performs the complete ordinary Standard audit, including its own threat map, investigation, source-backed validation, and attack-path reasoning, saving worker-bound checkpoints as results arrive and one final semantic scan draft when its audit finishes. The coordinator aggregates the finished Standard results and writes the parent scan's unsealed `scan-manifest.json`, `findings.json`, and `coverage.json` before returning `{ manifestPath }`. +Use `start_codex_security_deep_scan` to run repeated independent workers against the exact requested target and scope. Each worker reads `../../references/core-scan.md` directly and completes the ordinary Standard audit, saving checkpoints as results arrive and a final scan draft when the audit finishes. -The configured directories and exclusions remain in the scan scope and report. Reducers aggregate findings without coverage inputs or outputs. The coordinator supplies the parent `coverage.json` for compatibility from the configured scope and execution outcome; successful aggregates contain no surface observations, deferred work, or coverage notes. Each Standard worker retains its own coverage in its original result. +The coordinator combines the finished findings and writes the parent scan's unsealed `scan-manifest.json`, `findings.json`, and `coverage.json` before returning `{ manifestPath }`. The final report identifies the configured directories and exclusions alongside the findings. ## Phase Ownership diff --git a/plugins/codex-security/tests/test_deep_scan_successful_publication.py b/plugins/codex-security/tests/test_deep_scan_successful_publication.py index 84e89168f..5dceb3267 100644 --- a/plugins/codex-security/tests/test_deep_scan_successful_publication.py +++ b/plugins/codex-security/tests/test_deep_scan_successful_publication.py @@ -66,8 +66,7 @@ def create(*, mode="deep", scope="."): "SELECT started_at FROM scans WHERE id = ?", (scan_id,) ).fetchone()[0] if mode == "deep": - # The coordinator has already accepted and submitted its final aggregate. - # Exercise publication independently of the worker execution lifecycle. + # Start with a finished Deep result so these tests only need to save it. with workbench_db: workbench_db.execute( "INSERT INTO deep_scan_runs (scan_id, schema_version, workflow_version, " @@ -104,7 +103,7 @@ def create(*, mode="deep", scope="."): coverage["openQuestions"] = [{"question": "Which deployment controls apply?"}] for field in ("documentType", "schemaVersion", "scanId"): coverage.pop(field) - # Match the ordinary semantic envelopes written by the host draft writer. + # Use the same draft format as the writer, before finalization adds metadata. manifest = {"scan": {key: manifest["scan"][key] for key in ("target", "scope")}} findings[0]["severity"]["changeConditions"] = "Reassess if the upload route is removed." findings[0]["provenance"]["sourceFindings"] = [ From 28e6e82d1450ddd25e5b26c42b9ffd71b804781d Mon Sep 17 00:00:00 2001 From: Dane Schneider <269480009+daneschneider-oai@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:20:12 -0700 Subject: [PATCH 09/10] refactor(deep-scan): trim duplicate helpers and tests --- .../mcp-app/src/artifact-deep-reducer.ts | 11 +--- .../src/deep-scan/artifact-validation.ts | 3 +- .../tests/test_artifact_deep_reducer.mjs | 63 ++++++------------- .../tests/test_artifact_scan_draft.mjs | 31 ++++----- .../test_deep_scan_artifact_validation.mjs | 43 ------------- .../scripts/workbench_saved_results.py | 4 +- 6 files changed, 35 insertions(+), 120 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts b/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts index a2400d002..b8e147ed9 100644 --- a/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts +++ b/plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts @@ -23,6 +23,7 @@ import { parseDeepReduction, reconcileDeepReduction, type DeepReductionInput, + type DeepReductionSources, } from "./deep-scan/artifact-validation.js"; const schemaDocuments = [ @@ -43,14 +44,6 @@ export const deepReductionInputSchema = loadArtifactZodSchema( "reductionInput" ) as ZodType; -interface DeepReducerInputs { - discoveries: { - workerId: string; - result: DeepReductionInput; - }[]; - previous: DeepReductionInput | null; -} - interface BoundReducer { artifacts: DeepScanArtifacts; state: DeepReducerContext; @@ -61,7 +54,7 @@ interface BoundReducer { /** Read the findings and scan context assigned to this reducer. */ export async function getCodexSecurityDeepReducerInputs( context: ArtifactContext -): Promise { +): Promise { return withLogicalReducerErrors(context, async () => { const bound = bindDeepReducer(context); const discoveries = await Promise.all(bound.state.claimedWorkers.map(async (worker) => { diff --git a/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts b/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts index 9f884ce2e..291557a9e 100644 --- a/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts +++ b/plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts @@ -30,9 +30,8 @@ export function parseDeepReduction( input: Record, persisted = false, ): DeepReductionInput { - const { coverage: _legacyCoverage, ...reduction } = input; const standard = { - ...reduction, + ...input, coverage: { completeness: "complete", surfaces: [], diff --git a/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs b/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs index 208d76dc0..5466999cc 100644 --- a/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs +++ b/plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs @@ -56,12 +56,24 @@ try { const shared = finding("shared", "src/shared.ts"); const independent = finding("independent", "src/independent.ts"); + const rejectedCoverage = { + completeness: "partial", + surfaces: [ + { label: "SQL route", disposition: "rejected", notes: "Parameterized queries prevent injection.", + receiptRefs: ["artifacts/missing-worker-receipt.md"] }, + { label: "Archive upload", disposition: "needs_follow_up", notes: "The guard still needs review." }, + ], + explicitExclusions: [{ pattern: "vendor", reason: "Outside the requested source scope." }], + deferred: [{ candidateId: "candidate-upload", reason: "Review the guard.", paths: ["src/upload.ts"] }], + openQuestions: ["Does the alternate upload handler use the guard?"], + }; const first = await createWorker({ workersRoot, label: "discovery-0001", id: "worker-001", result: workerDraft([shared], { - threatModel: { summary: "Requests may reach shared code." } + threatModel: { summary: "Requests may reach shared code." }, + coverage: rejectedCoverage, }), completionSequence: 1 }); @@ -70,10 +82,14 @@ try { label: "discovery-0002", id: "worker-002", result: workerDraft([shared, independent], { - scope: { summary: "Shared and independent request handling." } + scope: { summary: "Shared and independent request handling." }, + coverage: { ...workerDraft([]).coverage, completeness: "unknown" }, }), completionSequence: 2 }); + const originalWorkerArtifacts = await Promise.all( + [first, second].map((worker) => readFile(worker.resultPath, "utf8")), + ); const outputRoot = path.join(dedupRoot, "dedup-0001", "output"); await mkdir(outputRoot, { recursive: true }); const context = { @@ -163,49 +179,8 @@ try { "reducer checkpoints retain the accepted findings and scope without coverage", ); - const rejectedCoverage = { - completeness: "partial", - surfaces: [ - { label: "SQL route", disposition: "rejected", notes: "Parameterized queries prevent injection.", - receiptRefs: ["artifacts/missing-worker-receipt.md"] }, - { label: "Archive upload", disposition: "needs_follow_up", notes: "The guard still needs review." }, - ], - explicitExclusions: [{ pattern: "vendor", reason: "Outside the requested source scope." }], - deferred: [{ candidateId: "candidate-upload", reason: "Review the guard.", paths: ["src/upload.ts"] }], - openQuestions: ["Does the alternate upload handler use the guard?"], - }; - const coverageWorker = await createWorker({ - workersRoot, label: "discovery-coverage", id: "worker-coverage", - result: workerDraft([], { coverage: rejectedCoverage }), completionSequence: 4, - }); - const unknownCoverageWorker = await createWorker({ - workersRoot, label: "discovery-unknown", id: "worker-unknown", - result: workerDraft([], { coverage: { ...workerDraft([]).coverage, completeness: "unknown" } }), - completionSequence: 5, - }); - const coverageWorkers = [coverageWorker, unknownCoverageWorker]; - const originalWorkerArtifacts = await Promise.all( - coverageWorkers.map((worker) => readFile(worker.resultPath, "utf8")), - ); - const coverageRoot = path.join(dedupRoot, "dedup-coverage", "output"); - await mkdir(coverageRoot, { recursive: true }); - const coverageContext = { - ...context, root: coverageRoot, - deepReducer: { scanRoot, claimedWorkers: coverageWorkers }, - }; - assert.deepEqual( - (await getCodexSecurityDeepReducerInputs(coverageContext)).discoveries, - coverageWorkers.map((worker) => ({ workerId: worker.id, result: withSourceRefs(worker) })), - "worker coverage is absent from model-visible reducer inputs", - ); - await recordCodexSecurityDeepReduction(coverageContext, reduction([])); - assert.deepEqual( - JSON.parse(await readFile(path.join(coverageRoot, "result.json"), "utf8")), - reduction([]), - "accepted reductions omit coverage even when every worker is partial or unknown", - ); assert.deepEqual( - await Promise.all(coverageWorkers.map((worker) => readFile(worker.resultPath, "utf8"))), + await Promise.all([first, second].map((worker) => readFile(worker.resultPath, "utf8"))), originalWorkerArtifacts, "reduction must not rewrite raw Standard worker coverage evidence", ); diff --git a/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs b/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs index acfbbd904..72f815fc3 100644 --- a/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs +++ b/plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs @@ -561,7 +561,11 @@ try { await readFile(path.join(deepParentRoot, "checkpoints", name), "utf8"), ]), ); - const acceptedDeepDraft = { ...input, complete: true }; + const acceptedDeepDraft = { + ...input, + complete: true, + coverage: { completeness: "complete", surfaces: [], explicitExclusions: [], deferred: [] }, + }; await recordCodexSecurityScanDraft(deepParentContext, acceptedDeepDraft); const acceptedDeepFindings = await readJson(deepParentRoot, "findings.json"); const acceptedDeepCoverage = await readJson(deepParentRoot, "coverage.json"); @@ -571,10 +575,11 @@ try { assert.equal(acceptedDeepCoverage.completeness, "complete"); assert.deepEqual(acceptedDeepCoverage.deferred, []); assert.deepEqual( - acceptedDeepCoverage.surfaces.map(({ label, disposition }) => ({ label, disposition })), - [{ label: "Archive extraction", disposition: "reported" }], + acceptedDeepCoverage.surfaces, + [], "accepted Deep coverage does not inherit obsolete parent review work", ); + assert.deepEqual(acceptedDeepCoverage.explicitExclusions, []); assert.deepEqual(acceptedDeepCoverage.includePaths, ["src", "lib"]); assert.deepEqual(acceptedDeepCoverage.excludePaths, ["vendor"]); assert.deepEqual(acceptedDeepManifest.scan.scope.includePaths, ["src", "lib"]); @@ -585,24 +590,10 @@ try { const obsoleteCheckpointPath = path.join(deepParentRoot, "checkpoints", "obsolete.json"); await writeFile(obsoleteCheckpointPath, "{malformed obsolete checkpoint\n"); - let deepPublicationCount = 0; - await recordCodexSecurityScanDraft( - deepParentContext, - acceptedDeepDraft, - async (draft, expectedDigest) => { - deepPublicationCount += 1; - assert.equal(expectedDigest, undefined); - assert.deepEqual(draft.findings, acceptedDeepFindings); - assert.deepEqual(draft.coverage, acceptedDeepCoverage); - }, - ); - assert.equal(deepPublicationCount, 1, "obsolete malformed checkpoints cannot block accepted Deep publication"); - - await writeFile(obsoleteCheckpointPath, ""); let deepWorkbenchWrites = 0; await recordCodexSecurityScanDraftViaWorkbench( deepParentContext, - input, + acceptedDeepDraft, async (arguments_) => { deepWorkbenchWrites += 1; assert.deepEqual(arguments_.slice(0, 3), ["write-scan-draft", "--scan-id", scanId]); @@ -614,11 +605,11 @@ try { const stagedCheckpoint = JSON.parse(await readFile(checkpointPath, "utf8")); assert.deepEqual(staged.findings, acceptedDeepFindings); assert.deepEqual(staged.coverage, acceptedDeepCoverage); - assert.deepEqual(stagedCheckpoint.findings, input.findings); + assert.deepEqual(stagedCheckpoint.findings, acceptedDeepDraft.findings); assert.equal(stagedCheckpoint.handoffClaimToken, undefined); }, ); - assert.equal(deepWorkbenchWrites, 1, "terminal Deep drafts still publish through the workbench lock despite obsolete empty checkpoints"); + assert.equal(deepWorkbenchWrites, 1, "terminal Deep drafts still publish through the workbench lock despite obsolete malformed checkpoints"); assert.deepEqual(await readdir(path.join(deepParentRoot, "drafts")), []); const pendingRoot = path.join(root, "pending-worker"); diff --git a/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs b/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs index 9147137bf..2b0bd4391 100644 --- a/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs +++ b/plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs @@ -306,49 +306,6 @@ async function testReducerValidation(root) { riskArea: "filesystem", notes: "The reducer completed the extraction review.", }; - await writeResult(resultPath, draft([firstFinding], { - coverage: { - completeness: "invalid legacy value", - surfaces: null, - deferred: "invalid legacy collection", - }, - })); - const validatedCoverage = await validateReducerArtifacts({ - artifacts, - artifactDir, - resultPath, - reducerId: "dedup-updated-coverage", - sources: { - discoveries: [{ - workerId: first.id, - result: draft([firstFinding], { - coverage: { - completeness: "partial", - surfaces: [ - { - riskArea: "filesystem", - notes: "The discovery worker still needed runtime validation.", - disposition: "needs_follow_up", - label: "Archive extraction", - }, - { label: "Worker-only handler", disposition: "needs_follow_up" }, - ], - explicitExclusions: [], - deferred: [{ reason: "An independent worker suggested another review." }], - }, - }), - }], - previous: null, - }, - }, scanId); - const reconciledResult = JSON.parse(await readFile(resultPath, "utf8")); - assert.equal(Object.hasOwn(reconciledResult, "coverage"), false); - assert.deepEqual( - validatedCoverage.result, - reconciledResult, - "accepted reducer results omit both malformed legacy coverage and worker coverage", - ); - assert.deepEqual(reconciledResult.findings[0].provenance.sourceFindingIds, ["worker-001:0"]); await writeResult(resultPath, draft([])); await assert.rejects( diff --git a/plugins/codex-security/scripts/workbench_saved_results.py b/plugins/codex-security/scripts/workbench_saved_results.py index c20774487..f4887006c 100644 --- a/plugins/codex-security/scripts/workbench_saved_results.py +++ b/plugins/codex-security/scripts/workbench_saved_results.py @@ -155,8 +155,8 @@ def _read_saved_result(scan_dir: Path, relative: str, scan_id: str) -> tuple[dic draft = _read_scan_local_json(scan_dir, relative, "Saved scan checkpoint") if draft.get("scanId") != scan_id: raise ContractError("checkpoint belongs to a different scan") - if not isinstance(draft.get("findings"), list) or ( - "coverage" in draft and not isinstance(draft["coverage"], dict) + if not isinstance(draft.get("findings"), list) or not isinstance( + draft.get("coverage", {}), dict ): raise ContractError("checkpoint has no semantic findings or has invalid coverage") return draft, _digest(draft) From 59b23a67eb0b92a978be5808cd0bcc3d409bd530 Mon Sep 17 00:00:00 2001 From: Dane Schneider <269480009+daneschneider-oai@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:40:24 -0700 Subject: [PATCH 10/10] fix(deep-scan): preserve checkpoint coverage requirements --- .../scripts/workbench_saved_results.py | 57 ++++++++------ .../tests/test_deep_scan_stop_conditions.py | 4 +- .../test_deep_scan_successful_publication.py | 74 ++++++++++++++++++- 3 files changed, 112 insertions(+), 23 deletions(-) diff --git a/plugins/codex-security/scripts/workbench_saved_results.py b/plugins/codex-security/scripts/workbench_saved_results.py index f4887006c..8c015a65d 100644 --- a/plugins/codex-security/scripts/workbench_saved_results.py +++ b/plugins/codex-security/scripts/workbench_saved_results.py @@ -112,13 +112,13 @@ def _latest_successful_reducer(workers: list[Any]) -> Any | None: ) -def _saved_result_paths(scan_dir: Path, workers: list[Any]) -> Iterator[str]: +def _saved_result_paths(scan_dir: Path, workers: list[Any]) -> Iterator[tuple[str, str | None]]: latest_reducer = _latest_successful_reducer(workers) - def checkpoints(directory: str) -> Iterator[str]: + def checkpoints(directory: str, kind: str | None = None) -> Iterator[tuple[str, str | None]]: for name in _children(scan_dir, directory): if re.fullmatch(r"[0-9a-f]{64}\.json", name): - yield f"{directory}/{name}" + yield f"{directory}/{name}", kind yield from checkpoints("checkpoints") for worker in workers: @@ -137,28 +137,33 @@ def checkpoints(directory: str) -> Iterator[str]: if re.fullmatch(r"attempt-\d+", name) ] for directory in directories: - checkpoint_paths = list(checkpoints(f"{directory}/checkpoints")) + checkpoint_paths = list(checkpoints(f"{directory}/checkpoints", worker["kind"])) if worker["kind"] == "discovery" or checkpoint_paths: - yield f"{directory}/result.json" + yield f"{directory}/result.json", worker["kind"] yield from checkpoint_paths if worker["result_manifest_path"] and ( worker["kind"] == "discovery" or (latest_reducer is not None and worker["id"] == latest_reducer["id"]) ): try: - yield Path(worker["result_manifest_path"]).relative_to(scan_dir).as_posix() + yield ( + Path(worker["result_manifest_path"]).relative_to(scan_dir).as_posix(), + worker["kind"], + ) except ValueError: continue -def _read_saved_result(scan_dir: Path, relative: str, scan_id: str) -> tuple[dict[str, Any], str]: +def _read_saved_result( + scan_dir: Path, relative: str, scan_id: str, *, kind: str | None = None +) -> tuple[dict[str, Any], str]: draft = _read_scan_local_json(scan_dir, relative, "Saved scan checkpoint") if draft.get("scanId") != scan_id: raise ContractError("checkpoint belongs to a different scan") if not isinstance(draft.get("findings"), list) or not isinstance( - draft.get("coverage", {}), dict + draft.get("coverage", {} if kind == "dedup" else None), dict ): - raise ContractError("checkpoint has no semantic findings or has invalid coverage") + raise ContractError("checkpoint has no semantic findings or coverage") return draft, _digest(draft) @@ -209,13 +214,13 @@ def _saved_results_changed(db: Any, connection: Any, scan: Any) -> bool: "FROM deep_scan_workers WHERE scan_id = ?", (scan["id"],), ).fetchall() - paths = set(_saved_result_paths(scan_dir, workers)) + paths = dict(_saved_result_paths(scan_dir, workers)) frozen_sources = scan["retained_source_digests_json"] def has_saved_source() -> bool: for path in paths: try: - _read_saved_result(scan_dir, path, scan["id"]) + _read_saved_result(scan_dir, path, scan["id"], kind=paths[path]) return True except (ContractError, OSError, ValueError): continue @@ -246,7 +251,9 @@ def has_saved_source() -> bool: current_sources = dict(published_sources) for path in paths: try: - _, current_sources[path] = _read_saved_result(scan_dir, path, scan["id"]) + _, current_sources[path] = _read_saved_result( + scan_dir, path, scan["id"], kind=paths[path] + ) except (ContractError, OSError, ValueError): continue return current_sources != published_sources @@ -292,23 +299,26 @@ def _recovery_source_digests(db: Any, connection: Any, scan: Any) -> tuple[dict[ else: include_parent = True + workers = connection.execute( + "SELECT id, kind, status, completed_at, artifact_dir, result_manifest_path " + "FROM deep_scan_workers WHERE scan_id = ?", + (scan["id"],), + ).fetchall() + paths = dict(_saved_result_paths(scan_dir, workers)) recovery_sources = dict(frozen_sources or {}) for relative, expected_digest in recovery_sources.items(): try: - _, digest = _read_saved_result(scan_dir, relative, scan["id"]) + _, digest = _read_saved_result(scan_dir, relative, scan["id"], kind=paths.get(relative)) except (ContractError, OSError, ValueError) as exc: raise ContractError("Frozen stopped-scan checkpoint set is incomplete.") from exc if digest != expected_digest: raise ContractError("checkpoint changed after the scan stopped") - workers = connection.execute( - "SELECT id, kind, status, completed_at, artifact_dir, result_manifest_path " - "FROM deep_scan_workers WHERE scan_id = ?", - (scan["id"],), - ).fetchall() - for relative in set(_saved_result_paths(scan_dir, workers)) - recovery_sources.keys(): + for relative in paths.keys() - recovery_sources.keys(): try: - _, recovery_sources[relative] = _read_saved_result(scan_dir, relative, scan["id"]) + _, recovery_sources[relative] = _read_saved_result( + scan_dir, relative, scan["id"], kind=paths[relative] + ) except (ContractError, OSError, ValueError): continue return recovery_sources, include_parent @@ -491,6 +501,7 @@ def merge_saved_results( parent_preserved_sources = recorded source_digests.update(parent_preserved_sources) paths: dict[str, str | None] = {} + reducer_paths: set[str] = set() current_results: set[str] = set() reducer_outputs: list[tuple[Any, str, list[str], int]] = [] reducer = _latest_successful_reducer(workers) @@ -499,6 +510,7 @@ def merge_saved_results( try: latest_reducer = Path(reducer["result_manifest_path"]).relative_to(scan_dir).as_posix() paths[latest_reducer] = None + reducer_paths.add(latest_reducer) except ValueError: warnings.append("Skipped a reducer result outside the scan directory.") @@ -528,6 +540,7 @@ def reducer_output(directory: str, attempt: int, reducer_worker: Any) -> None: paths[result_path] = None for checkpoint_path in checkpoint_paths: paths[checkpoint_path] = None + reducer_paths.update([result_path, *checkpoint_paths]) reducer_outputs.append((reducer_worker, result_path, checkpoint_paths, attempt)) reducer_output(output, int(worker["attempt"] or 0), worker) @@ -572,7 +585,9 @@ def reducer_output(directory: str, attempt: int, reducer_worker: Any) -> None: for relative, worker_id in paths.items(): try: - draft, digest = _read_saved_result(scan_dir, relative, scan_id) + draft, digest = _read_saved_result( + scan_dir, relative, scan_id, kind="dedup" if relative in reducer_paths else None + ) if frozen_source_digests is not None and frozen_source_digests[relative] != digest: raise ContractError("checkpoint changed after the scan stopped") source_digests[relative] = digest diff --git a/plugins/codex-security/tests/test_deep_scan_stop_conditions.py b/plugins/codex-security/tests/test_deep_scan_stop_conditions.py index 5871eeedc..253297eab 100644 --- a/plugins/codex-security/tests/test_deep_scan_stop_conditions.py +++ b/plugins/codex-security/tests/test_deep_scan_stop_conditions.py @@ -8,7 +8,9 @@ add_worker, assert_published_aggregate, complete, - publication_scan, +) +from test_deep_scan_successful_publication import ( + publication_scan as publication_scan, ) diff --git a/plugins/codex-security/tests/test_deep_scan_successful_publication.py b/plugins/codex-security/tests/test_deep_scan_successful_publication.py index 5dceb3267..8a144ece4 100644 --- a/plugins/codex-security/tests/test_deep_scan_successful_publication.py +++ b/plugins/codex-security/tests/test_deep_scan_successful_publication.py @@ -292,12 +292,14 @@ def test_deep_prepare_and_complete_preserve_the_same_aggregate( [ ("standard-worker-checkpoint", ".", True), ("deep-reducer-checkpoint", ".", True), + ("deep-reducer-archived-checkpoint", ".", True), ("deep-reducer-result", ".", True), ("deep-reducer-result", "subdir", False), ], ids=[ "standard-worker-checkpoint", "deep-reducer-checkpoint", + "deep-reducer-archived-checkpoint", "deep-reducer-result", "scoped-reducer-without-parent", ], @@ -343,7 +345,12 @@ def test_stopped_deep_scan_still_salvages_saved_findings( checkpoint = None result.write_text(json.dumps(saved)) else: - checkpoint = write_checkpoint(result.parent / "checkpoints", saved) + checkpoint_root = ( + result.parent / "attempts" / "attempt-01" + if source == "deep-reducer-archived-checkpoint" + else result.parent + ) + checkpoint = write_checkpoint(checkpoint_root / "checkpoints", saved) result.write_text("{interrupted worker output") result_bytes = result.read_bytes() @@ -377,6 +384,71 @@ def test_stopped_deep_scan_still_salvages_saved_findings( assert result.read_bytes() == result_bytes +@pytest.mark.parametrize("source", ["result", "checkpoint", "parent-checkpoint"]) +def test_stopped_deep_scan_ignores_non_reducer_sources_without_coverage( + workbench_api, workbench_db, publication_scan, source +): + scan = publication_scan() + with workbench_db: + workbench_db.execute( + "UPDATE deep_scan_runs SET status = 'running', phase = 'discovery', " + "terminal_reason = NULL, completed_at = NULL WHERE scan_id = ?", + (scan.scan_id,), + ) + invalid_finding = copy.deepcopy(scan.findings[0]) + invalid_finding["identity"]["anchor"] = "non-reducer-without-coverage" + invalid_finding["summary"] = "Finding from a non-reducer artifact missing required coverage." + saved = { + "scanId": scan.scan_id, + "complete": source == "result", + "findings": [invalid_finding], + } + if source == "parent-checkpoint": + source_path = write_checkpoint(scan.scan_dir / "checkpoints", saved) + else: + result = add_worker( + workbench_db, scan, status="succeeded" if source == "result" else "running" + ) + if source == "result": + result.write_text(json.dumps(saved)) + source_path = result + else: + source_path = write_checkpoint(result.parent / "checkpoints", saved) + source_bytes = source_path.read_bytes() + source_relative = source_path.relative_to(scan.scan_dir).as_posix() + + stopped = workbench_api["fail_scan"]( + workbench_db, + Namespace( + scan_id=scan.scan_id, claim_token=None, cost_json=None, message="Scan interrupted." + ), + )["scan"] + + assert stopped["progress"]["status"] == "failed" + assert stopped["resultsRecoveryNeeded"] is False + findings = json.loads((scan.scan_dir / "findings.json").read_text())["findings"] + assert [finding["summary"] for finding in findings] == [scan.findings[0]["summary"]] + manifest = json.loads((scan.scan_dir / "scan-manifest.json").read_text()) + frozen = json.loads( + workbench_db.execute( + "SELECT retained_source_digests_json FROM scans WHERE id = ?", (scan.scan_id,) + ).fetchone()[0] + ) + assert manifest["scan"]["preservedSources"] == frozen + assert source_relative not in frozen + artifact_names = ("scan-manifest.json", "findings.json", "coverage.json") + published = {name: (scan.scan_dir / name).read_bytes() for name in artifact_names} + + recovered = workbench_api["recover_scan_results"]( + workbench_db, Namespace(scan_id=scan.scan_id) + )["scan"] + + assert recovered["findingCount"] == 1 + assert recovered["resultsRecoveryNeeded"] is False + assert {name: (scan.scan_dir / name).read_bytes() for name in artifact_names} == published + assert source_path.read_bytes() == source_bytes + + def test_standard_publication_preserves_deliberately_partial_coverage( workbench_api, workbench_db, publication_scan ):