fix(artifacts): keep saved findings consistent through recovery

plugins/codex-security/mcp-app/src/artifact-scan-draft.ts

@@ -84,6 +84,12 @@ export const completedScanInputSchema = loadArtifactZodSchema(
   "completedScanInput",
 ) as z.ZodType<CompletedScanInput>;
 
+const findingIdentitySchema = loadArtifactZodSchema(
+  schemaDocuments,
+  scanDraftDocument.$id,
+  "identity",
+);
+
 /** Replace the three existing final-input documents without completing or sealing a scan. */
 export async function recordCodexSecurityScanDraft(
   context: ArtifactContext,
@@ -499,6 +505,25 @@ async function preserveScanDraft(
       .map(({ input }) => input)
       .reverse();
     progressSources.push(input);
+    const terminalSurfaceIds = new Set(
+      (result.coverage.surfaces as JsonObject[])
+        .filter((row) => row.disposition !== "needs_follow_up")
+        .flatMap((row) =>
+          [row.id, row.candidateId].filter(
+            (id): id is string => typeof id === "string",
+          ),
+        ),
+    );
+    const terminalWorkIds = new Set([
+      ...terminalSurfaceIds,
+      ...closedIds,
+      ...terminalOutcomeIds,
+      ...(result.coverage.deferred as JsonObject[]).flatMap((row) =>
+        [row.id, row.candidateId].filter(
+          (id): id is string => typeof id === "string",
+        ),
+      ),
+    ]);
     let acceptProgress = coverageHasOutstandingWork(result.coverage);
     for (const observation of progressSources) {
       const progress = structuredClone(observation);
@@ -509,7 +534,15 @@ async function preserveScanDraft(
           ),
         ),
       );
-      if (reopenedIds.size > 0) acceptProgress = true;
+      const newWork = [
+        ...(progress.coverage.deferred as JsonObject[]),
+        ...(progress.coverage.surfaces as JsonObject[]).filter(
+          (row) => row.disposition === "needs_follow_up",
+        ),
+      ].some(
+        (row) => !terminalWorkIds.has((row.candidateId ?? row.id) as string),
+      );
+      if (reopenedIds.size > 0 || newWork) acceptProgress = true;
       if (!acceptProgress) continue;
       result.complete = false;
       const resolved = reconcileDeferredSurfaces(
@@ -521,19 +554,34 @@ async function preserveScanDraft(
         [],
       );
       for (const surface of resolved) reopenedSurfaces.add(surface);
+      const deferred = (progress.coverage.deferred as JsonObject[]).filter(
+        (row) =>
+          keepsGenericWork(row) ||
+          !terminalOutcomeIds.has((row.candidateId ?? row.id) as string),
+      );
       result.coverage = preserveScanCoverage(
         {
           ...result.coverage,
-          deferred: (progress.coverage.deferred as JsonObject[]).filter(
-            (row) =>
-              keepsGenericWork(row) ||
-              !terminalOutcomeIds.has((row.candidateId ?? row.id) as string),
-          ),
+          deferred,
           surfaces: (progress.coverage.surfaces as JsonObject[]).filter(
             (surface) =>
-              keepsGenericWork(surface) ||
-              (!terminalOutcomeIds.has(surface.id as string) &&
-                !terminalOutcomeIds.has(surface.candidateId as string)),
+              (keepsGenericWork(surface) ||
+                (!terminalOutcomeIds.has(surface.id as string) &&
+                  !terminalOutcomeIds.has(surface.candidateId as string))) &&
+              (surface.disposition !== "needs_follow_up" ||
+                !terminalSurfaceIds.has(
+                  (surface.candidateId ?? surface.id) as string,
+                ) ||
+                reopenedIds.has(
+                  (surface.candidateId ?? surface.id) as string,
+                ) ||
+                deferred.some(
+                  (row) =>
+                    row.id === surface.id ||
+                    ((row.surfaceIds as string[] | undefined) ?? []).includes(
+                      surface.id as string,
+                    ),
+                )),
           ),
         },
         [result.coverage],
@@ -630,18 +678,52 @@ async function preserveScanDraft(
         disposition.finding ??= structuredClone(finding);
         continue;
       }
+      if (
+        result.findings.some((current) => {
+          const history = (current.provenance as JsonObject).previousFindings;
+          return (
+            Array.isArray(history) &&
+            history.some(
+              (previous) =>
+                isObject(previous) && containsSavedFinding(previous, finding),
+            )
+          );
+        })
+      )
+        continue;
       const matches = result.findings.filter((current) =>
         sameSavedFinding(current, finding),
       );
+      const containing = matches.filter((current) =>
+        containsSavedFinding(current, finding),
+      );
+      const revisions = matches.filter((current) =>
+        sameSavedRevision(current, finding),
+      );
       if (
         matches.length === 1 &&
-        source.findings.filter((current) => sameSavedFinding(current, finding))
-          .length === 1
+        source.findings.filter((current) =>
+          sameSavedFinding(current, matches[0]!),
+        ).length === 1
       ) {
         preserveFindingDetails(matches[0]!, finding);
-      } else {
-        if (!matches.some((current) => containsSavedFinding(current, finding)))
-          result.findings.push(structuredClone(finding));
+      } else if (
+        containing.length === 1 &&
+        source.findings.filter((previous) =>
+          containsSavedFinding(containing[0]!, previous),
+        ).length === 1
+      ) {
+        // An unchanged sibling still owns its previously published identity.
+        preserveFindingDetails(containing[0]!, finding);
+      } else if (
+        revisions.length === 1 &&
+        source.findings.filter((current) =>
+          sameSavedRevision(current, revisions[0]!),
+        ).length === 1
+      ) {
+        preserveFindingDetails(revisions[0]!, finding);
+      } else if (containing.length === 0) {
+        result.findings.push(structuredClone(finding));
       }
     }
     const resolvedIds = new Set(
@@ -712,6 +794,7 @@ async function preserveScanDraft(
   result.coverage.surfaces = normalizeSurfaces(
     result.coverage.surfaces as JsonObject[],
   );
+  result = scanDraftInputSchema.parse(result);
   if (saveCheckpoint) await saveScanDraftCheckpoint(context, result);
   return { input: result, previousDigest: previousState.digest };
 }
@@ -1507,6 +1590,28 @@ function sameSavedFinding(left: JsonObject, right: JsonObject): boolean {
   if (left.ruleId !== right.ruleId) return false;
   if (left.identity && right.identity)
     return scanFindingIdentity(left) === scanFindingIdentity(right);
+  const leftExplicitCandidate = findingIdentityCandidate(left);
+  const rightExplicitCandidate = findingIdentityCandidate(right);
+  if (
+    leftExplicitCandidate &&
+    rightExplicitCandidate &&
+    leftExplicitCandidate !== rightExplicitCandidate
+  )
+    return false;
+  // Report identifiers distinguish siblings that share the same candidate.
+  const reportIds = ["reportId", "ledgerRowId"].map((field) =>
+    [left, right].map((finding) => {
+      const value = (finding.extensions as JsonObject | undefined)?.[field];
+      return typeof value === "string" && value.trim() ? value : undefined;
+    }),
+  );
+  if (
+    reportIds.some(([left]) => left) &&
+    reportIds.some(([, right]) => right) &&
+    (!reportIds.some(([left, right]) => left && right) ||
+      reportIds.some(([left, right]) => left && right && left !== right))
+  )
+    return false;
   const leftCandidate = findingCandidateId(left);
   if (leftCandidate && leftCandidate === findingCandidateId(right)) return true;
   return (
@@ -1515,6 +1620,14 @@ function sameSavedFinding(left: JsonObject, right: JsonObject): boolean {
   );
 }
 
+function sameSavedRevision(left: JsonObject, right: JsonObject): boolean {
+  return (
+    left.title === right.title &&
+    isDeepStrictEqual(left.locations, right.locations) &&
+    sameSavedFinding(left, right)
+  );
+}
+
 function withoutPreviousFindings(finding: JsonObject): JsonObject {
   const result = structuredClone(finding);
   if (isObject(result.provenance)) delete result.provenance.previousFindings;
@@ -1526,7 +1639,10 @@ export function preserveFindingDetails(
   current: JsonObject,
   previous: JsonObject,
 ): void {
-  if (current.identity === undefined && previous.identity !== undefined) {
+  if (
+    current.identity === undefined &&
+    findingIdentitySchema.safeParse(previous.identity).success
+  ) {
     current.identity = structuredClone(previous.identity);
   }
   const provenance = requireObject(
@@ -1751,6 +1867,16 @@ export function scanFindingIdentity(finding: JsonObject): string {
   ]);
 }
 
+function findingIdentityCandidate(finding: JsonObject): string | undefined {
+  return [
+    (finding.extensions as JsonObject | undefined)?.candidateId,
+    (finding.provenance as JsonObject | undefined)?.candidateId,
+  ].find(
+    (value): value is string =>
+      typeof value === "string" && Boolean(value.trim()),
+  );
+}
+
 function findingCandidateId(finding: JsonObject): string | undefined {
   const provenance = finding.provenance;
   if (
@@ -2260,12 +2386,8 @@ function buildScope(
 function buildFindings(findings: JsonObject[], mode?: string): JsonObject[] {
   const anchorCounts = new Map<string, number>();
   const anchors = findings.map((finding, index) => {
-    const candidateId = (finding.extensions as JsonObject | undefined)
-      ?.candidateId;
     const identitySource =
-      typeof candidateId === "string" && candidateId.trim()
-        ? candidateId
-        : (finding.title as string);
+      findingIdentityCandidate(finding) ?? (finding.title as string);
     const anchor =
       finding.identity === undefined
         ? semanticIdentifier(identitySource, `finding-${index + 1}`)
@@ -2301,15 +2423,22 @@ function buildFindings(findings: JsonObject[], mode?: string): JsonObject[] {
       identity,
     };
   });
-  if (mode !== "deep") return identified;
+  // Keep generated identities distinct without replacing an authored identity.
+  const authored = new Set(
+    findings
+      .filter((finding) => finding.identity !== undefined)
+      .map(scanFindingIdentity),
+  );
 
   // 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<string>();
-  return identified.map((finding) => {
+  return identified.map((finding, index) => {
+    const explicit = findings[index]!.identity !== undefined;
+    if (mode !== "deep" && explicit) return finding;
     const key = scanFindingIdentity(finding);
-    if (!used.has(key)) {
+    if (!used.has(key) && (explicit || !authored.has(key))) {
       used.add(key);
       return finding;
     }
@@ -2327,12 +2456,14 @@ function buildFindings(findings: JsonObject[], mode?: string): JsonObject[] {
       reserved.has(scanFindingIdentity(distinct)) ||
       used.has(scanFindingIdentity(distinct))
     );
-    const provenance = finding.provenance as JsonObject;
-    distinct.provenance = {
-      ...provenance,
-      preservedIdentity:
-        provenance.preservedIdentity ?? structuredClone(identity),
-    };
+    if (explicit) {
+      const provenance = finding.provenance as JsonObject;
+      distinct.provenance = {
+        ...provenance,
+        preservedIdentity:
+          provenance.preservedIdentity ?? structuredClone(identity),
+      };
+    }
     used.add(scanFindingIdentity(distinct));
     return distinct;
   });
@@ -2628,6 +2759,6 @@ function semanticIdentifier(value: string, fallback: string): string {
     .replace(/[\u0300-\u036f]/gu, "")
     .toLowerCase()
     .replace(/[^a-z0-9._/-]+/gu, "-")
-    .replace(/^-+|-+$/gu, "");
+    .replace(/^[^a-z0-9]+|-+$/gu, "");
   return identifier || fallback;
 }

plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.ts

@@ -242,19 +242,19 @@ try {
     "checkpoints",
     acceptedHead.checkpoint,
   );
-  assert.notEqual(
+  assert.equal(
     acceptedHeadDraft.complete,
     false,
-    "a rejected incomplete write must not become the authoritative checkpoint head",
+    "new late work must remain incomplete in the authoritative checkpoint",
   );
   assert.deepEqual(acceptedHeadDraft.findings, [finding]);
   const acceptedResult = await readJson(rejectedIncompleteRoot, "result.json");
   assert.equal(
     acceptedResult.coverage.deferred.some(
       (item: FixtureFinding) => item.candidateId === "late-incomplete-review",
     ),
-    false,
-    "a rejected incomplete write must not change the completed result",
+    true,
+    "new late work must survive alongside the completed candidate",
   );
 
   const deferredDoesNotRejectContext = await createWorkerContext(
@@ -2477,13 +2477,13 @@ try {
     ),
     [
       { anchor: "candidate-authored-instance", instance: "dss-147-a" },
-      { anchor: "candidate-authored-instance", instance: "dss-147-a" },
+      { anchor: "candidate-authored-instance", instance: "dss-147-a-2" },
       {
         anchor: "candidate-authored-instance",
         instance: "ledger-row-c",
       },
     ],
-    "duplicate stable instance sources remain collisions for finalization",
+    "generated instances avoid an existing authored identity",
   );
 
   const collisionFindings = [
@@ -2550,10 +2550,17 @@ try {
     assert.deepEqual(
       standardFindings.map((item: FixtureFinding) => item.identity),
       collisionCase.findings.map(
-        (item: FixtureFinding) =>
-          item.identity ?? collisionCase.originalIdentity,
+        (item: FixtureFinding, index) =>
+          item.identity ?? {
+            ...collisionCase.originalIdentity,
+            ...(index === 0
+              ? {}
+              : {
+                  instance: `${(collisionCase.originalIdentity as { instance?: string }).instance}-${index + 1}`,
+                }),
+          },
       ),
-      `${collisionCase.label} identity collisions retain the existing Standard shape`,
+      `${collisionCase.label} identities preserve authored values and split generated collisions`,
     );
     assert.deepEqual(
       standardFindings.map((item: FixtureFinding) => item.provenance),
@@ -2587,7 +2594,7 @@ try {
     assert.deepEqual(
       deepFindings.map((item: FixtureFinding) => item.provenance),
       collisionCase.findings.map((item: FixtureFinding, index) =>
-        index === 1 || index === 2
+        item.identity !== undefined && (index === 1 || index === 2)
           ? {
               ...item.provenance,
               preservedIdentity: collisionCase.originalIdentity,