fix: salvage optional finding fields instead of discarding the finding

[Haiduongcable]

Summary

finalize_scan_contract.py already salvages a malformed writeup, remediationTests, or
preventiveControls field by dropping just that field and keeping the finding. Any other schema
mismatch falls through to the outer handler and discards the entire finding instead.

Two concrete cases hit this even though the finding's actual content is otherwise fine:

• taxonomy.cwe is optional in the hand-rolled validator (defaults to an empty list) but required
  as a key by the JSON schema, so a finding missing the key outright is silently dropped in full.
• codeEvidence is optional at the finding level, but a single malformed entry (missing
  explanation, wrong shape, missing id, duplicate id, etc.) currently fails the whole finding
  instead of just that auxiliary field, unlike its sibling optional fields.

A real, otherwise-valid finding can vanish from report.md, findings.json, SARIF, and CSV because
one optional field was malformed, with only a generic completion warning as a trace.

Changes

• Backfill a missing taxonomy.cwe key to an empty list before final schema validation, matching
  the hand-rolled validator's own tolerant default.
• Extend the existing auxiliary-field salvage pattern to codeEvidence: a malformed entry drops
  just that field, keeping the finding.
• Move codeEvidence recovery ahead of _validate_finding. The hand-rolled validator independently
  rejects some codeEvidence malformations (non-array, non-object entries, missing id, duplicate
  id) before the schema-based recovery would otherwise run, so validating/stripping codeEvidence
  first ensures every malformation type is salvaged consistently, not only the ones a bare
  JSON-schema check would catch.
• Dropping codeEvidence can leave dangling evidenceRefs in rootCause, validation, and
  attackPath — including the documented nested attackPath.dataflow.evidenceRefs location. These
  are pruned in the same pass, with a recovery warning recorded, so the sealed contract never
  references removed evidence.

Deliberately not changed

identity and confidence.rationale remain hard requirements in both validators. There is no safe
default for identity, and confidence.rationale is a genuine content requirement rather than a
validator mismatch, so a finding missing either is still (correctly) discarded.

Verification

Reproduced directly against finalize_scan_contract.py and the bundled example finding before any
change: a missing taxonomy.cwe and five distinct malformed-codeEvidence shapes each discarded
the whole finding. After the fix, all seven cases are recovered — finding kept, offending
field/reference stripped, warning recorded — while identity/confidence.rationale cases are
unchanged.

Added a regression case to tests-ts/scan-recovery.test.ts covering all of the above, including the
nested attackPath.dataflow.evidenceRefs case. Confirmed the test fails against unpatched
upstream/main source (findingCount 4 → 1, all three malformed findings fully discarded) before
passing against the fix.

Full local gate: pnpm run types, pnpm run test (702 pass / 5 skip, 0 fail), pnpm run format,
pnpm run build, and the packaging check (pnpm pack + check:package) all pass.

[Haiduongcable]

Fork CI is green on this exact commit, run within the fork so the full matrix executes without
requiring workflow approval: Haiduongcable#3

Commit: e09e0dbdeeff8e9d40a07afe4f2967ed99cd4af3

Check	Result	Duration
ubuntu-latest / node-22	pass	1m27s
ubuntu-latest / node-24	pass	1m30s
ubuntu-latest / node-24.0.0	pass	1m45s
ubuntu-latest / node-26	pass	1m44s
ubuntu-latest / node-26.0.0	pass	1m41s
macos-latest / node-22	pass	2m41s
windows-latest / node-22	pass	4m52s
linux-amd64	pass	1m27s

Local verification (sdk/typescript):

• pnpm run types — clean
• pnpm run test — 702 pass / 5 skip / 0 fail (701 pass / 5 skip on unpatched upstream/main; the
  new regression case fails against unpatched source before passing against this change)
• pnpm run format — clean
• pnpm run build — clean
• pnpm pack + pnpm run check:package — clean

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e09e0dbdee

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[riju568]

Issue & Root Cause Analysis Problem When there is a little bit of the schema mismatch on the optional fields (taxonomy.cwe not exists or garbage value is there for elements in codeEvidence), the whole findings have been completely dropped in finalizescancontract.py. The New bug noticed during code review When we try to prune orphaned evidenceRefs after cleaning out the ill-formed codeEvidence, we use the lookup on sets ref in knownevidenceids. If one of the elements of a list of evidenceRefs is not hashable (due to ill-formed LLM or scanner's garbage output contains either dictionary/object or a nested list).

Then we got TypeError.

As we have only wrapped contract checks within ContractError handler, this new type of error will crack the cli application down and leave us with trace backs.

Python logic after changes (finalizescancontract.py) In the references pruning logic used for root Cause, validation and attack Path (including in attackPath.dataflow), remove such references:
in the logic change

V

known_evidence_ids = {
ev["id"]
for ev in finding.get("codeEvidence", [])
if isinstance(ev, dict) and isinstance(ev.get("id"), str)
}

def prune_evidence_refs(container: dict) -> bool:
"""Safely prune unhashable or missing evidence references from a container dict."""
refs = container.get("evidenceRefs")
if not isinstance(refs, list):
return False

modified = False
kept_refs = []
for ref in refs:
    # Check that ref is a string before checking set membership to prevent TypeError
    if isinstance(ref, str) and ref in known_evidence_ids:
        kept_refs.append(ref)
    else:
        modified = True

if modified:
    if kept_refs:
        container["evidenceRefs"] = kept_refs
    else:
        container.pop("evidenceRefs", None)

return modified


-----------------------------------------------------------------------

[riju568]

known_evidence_ids = {
ev["id"]
for ev in finding.get("codeEvidence", [])
if isinstance(ev, dict) and isinstance(ev.get("id"), str)
}

def prune_evidence_refs(container: dict) -> bool:
"""Safely prune unhashable or missing evidence references from a container dict."""
refs = container.get("evidenceRefs")
if not isinstance(refs, list):
return False

modified = False
kept_refs = []
for ref in refs:
# Check that ref is a string before checking set membership to prevent TypeError
if isinstance(ref, str) and ref in known_evidence_ids:
kept_refs.append(ref)
else:
modified = True

if modified:
if kept_refs:
container["evidenceRefs"] = kept_refs
else:
container.pop("evidenceRefs", None)

return modified

finalize_scan_contract.py

[Haiduongcable]

Addressed the automated review finding above (P1: unhashable evidence references before set lookup).

Root cause: the dangling-evidenceRefs pruning added alongside codeEvidence recovery tested
set membership directly — ref in known_evidence_ids — without first checking that each reference
was a string. known_evidence_ids is a Python set, and an evidenceRefs entry that is a JSON
object or array is unhashable, so the membership test itself raises TypeError. The recovery loop
and the CLI are only prepared to catch ContractError, so one malformed optional reference aborted
scan completion with a raw traceback instead of being recovered or discarded like every other
malformed field on this path.

Fix: guard the membership test with isinstance(ref, str) first, so non-string entries — an
object, an array, or any other non-string but otherwise-hashable value — are pruned the same way a
missing or unknown reference already is, instead of crashing:

kept_refs = [
    ref for ref in refs if isinstance(ref, str) and ref in known_evidence_ids
]

Verification:

• Reproduced the exact crash directly against _recover_unsealed_findings before making any change:
  a codeEvidence entry malformed enough to be stripped, plus an evidenceRefs list containing a
  dict, raised TypeError: unhashable type: 'dict', uncaught.
• Added a regression case to tests-ts/scan-recovery.test.ts covering non-string evidenceRefs
  entries (object, array, and a hashable-but-non-string value). Confirmed it fails against the
  pre-fix commit with the same uncaught TypeError propagating out of the workbench subprocess,
  then passes against the fix.
• Full local gate: pnpm run types, pnpm run test (708 pass / 5 skip, 0 fail), pnpm run format,
  pnpm run build all pass.
• Fork CI green on 7a8f60ecc1e56ff685a057e7c58befd9623a51e6:

Check	Result	Duration
ubuntu-latest / node-22	pass	1m46s
ubuntu-latest / node-24	pass	1m44s
ubuntu-latest / node-24.0.0	pass	1m37s
ubuntu-latest / node-26	pass	1m46s
ubuntu-latest / node-26.0.0	pass	1m44s
macos-latest / node-22	pass	1m57s
windows-latest / node-22	pass	4m51s
linux-amd64	pass	1m40s

(Haiduongcable#3)

[Haiduongcable]

@riju568 thanks for the writeup and the proposed patch — the isinstance guard was exactly the
right direction, and having the failure mode spelled out that clearly made it quick to reproduce and
confirm before fixing. Addressed above, with a regression test that fails against the pre-fix commit
and a fresh green CI run.

[riju568]

Thanks for your appreciation. @Haiduongcable I am hare if anything I can contribute. : )

[riju568]

known_evidence_ids = {
ev["id"]
for ev in finding.get("codeEvidence", [])
if isinstance(ev, dict) and isinstance(ev.get("id"), str) and ev["id"].strip()
}

def prune_evidence_refs_recursive(data) -> bool:
"""recursively, de-clutter Evidence, where Unmaped in Evidence is not contained (e.g., empty or unmatched) and does not comply to (dict or list)-type in nature, can take this. True if a change happened.
"""
modified = False

if isinstance(data, dict):
    if "evidenceRefs" in data:
        refs = data["evidenceRefs"]
        if not isinstance(refs, list):
            data.pop("evidenceRefs", None)
            modified = True
        else:
            # Case 2: Filter non-string, whitespace, or unmapped IDs
            kept_refs = []
            for ref in refs:
                if isinstance(ref, str) and ref.strip() and ref in known_evidence_ids:
                    kept_refs.append(ref)
                else:
                    modified = True
            if modified:
                if kept_refs:
                    data["evidenceRefs"] = kept_refs
                else:
                    data.pop("evidenceRefs", None)
    for key, value in list(data.items()):
        if key != "evidenceRefs":
            if prune_evidence_refs_recursive(value):
                modified = True

elif isinstance(data, list):
    for item in data:
        if prune_evidence_refs_recursive(item):
            modified = True

return modified

finalize_scan_contract.py

[riju568]

//tests-ts/scan-recovery.test.ts

it('Salves findings that have non-list evidenceRefs and steps that use array inputs for the attackpath', async () => {
const malformedFinding = {
id: 'FINDING-MALFORMED-REFS',
identity: 'rule-id-1',
confidence: { rationale: 'Valid rationale' },
codeEvidence: [{ id: 'EVD-1', path: 'src/index.ts' }],
rootCause: {
evidenceRefs: 'EVD-1', // Malformed: string instead of array
},
attackPath: [
{
step: 1,
evidenceRefs: ['EVD-1', 'EVD-NONEXISTENT', { invalid: 'object' }], // Mixed array
},
],
};

const result = await runScanContractRecovery([malformedFinding]);

expect(result.findings).toHaveLength(1);
const recovered = result.findings[0];

expect(recovered.rootCause.evidenceRefs).toBeUndefined();

expect(recovered.attackPath[0].evidenceRefs).toEqual(['EVD-1']);
expect(result.warnings).toContainEqual(
expect.stringMatching(/pruned invalid or dangling evidence references/i)
);
});

[riju568]

evidenceRefs Payload

refs = container.get("evidenceRefs")
if not isinstance(refs, list):
return False # Leaves the invalid field in place!

[mldangelo-oai]

Related finding-recovery work: merged #159 already handles malformed optional remediation fields, while #184 separately normalizes valid list-shaped severity change conditions. This PR addresses missing taxonomy.cwe, malformed codeEvidence, and dangling evidence references. These cases share the finalization/recovery code but are complementary rather than duplicates; please preserve separate regressions when coordinating the changes.

[Haiduongcable]

@mldangelo-oai thanks — the regressions are now individually preservable.

Split into one named test per failure mode, each with its own fixture and scan:

• backfills a missing taxonomy.cwe instead of discarding the finding
• drops schema-invalid codeEvidence and prunes the rootCause references it strands
• drops duplicate codeEvidence ids and prunes nested attackPath references

The last two stay separate because they hit different detection paths — a missing required field is caught by the JSON-schema pass, duplicate ids only by the hand-rolled validator — and strand references in different places (rootCause.evidenceRefs vs attackPath.evidenceRefs and nested attackPath.dataflow.evidenceRefs). Either can regress alone.

Confirmed the split is real, not cosmetic: restoring the upstream finalize_scan_contract.py fails all four recovery tests independently (16 pass / 4 fail). Coverage unchanged — the same six warning assertions, now 1 / 2 / 3.

Verification: types, format, build clean; 721 pass / 5 skip / 0 fail; check:package green. Current with main, no conflict with #184.

One flag while you coordinate: recovery warnings are appended before _validate_finding, so a finding recovered then discarded for an unrelated missing field leaves a "Recovered finding N" warning next to its skip notice — counts slightly overstate what was salvaged. Happy to buffer them until the finding validates, here or separately.