Skip to content

fix: preserve findings with list severity conditions - #184

Draft
mldangelo-oai wants to merge 1 commit into
mainfrom
mdangelo/codex/fix-finding-severity-change-conditions
Draft

fix: preserve findings with list severity conditions#184
mldangelo-oai wants to merge 1 commit into
mainfrom
mdangelo/codex/fix-finding-severity-change-conditions

Conversation

@mldangelo-oai

Copy link
Copy Markdown
Collaborator

Summary

  • Normalize a non-empty array of valid severity change-condition strings into the canonical prose string during unsealed finding recovery.
  • Continue rejecting empty arrays, blank entries, and non-string entries so malformed finding data remains fail-closed.
  • Document the producer-side mapping from attack_path.change_conditions to the schema-required severity.changeConditions string.

Why

Attack-path analysis can naturally emit several severity-change conditions as an array, while the canonical findings schema accepts only a non-empty string. Before this change, an otherwise valid finding with that producer shape was discarded during completion. If every finding had the same shape, the finished scan incorrectly reported zero findings and downgraded complete coverage to partial.

Recovery now joins valid condition entries into one prose string before validation, preserves every independently validated finding, records an explicit normalization warning, and retains complete coverage. Invalid arrays still follow the existing strict rejection path.

Validation

  • Full SDK suite: 708 passed, 5 expected skips, 0 failed.
  • Focused scan recovery suite: 18 passed, 0 failed.
  • pnpm run types.
  • pnpm run format.
  • git diff --check.
  • Reconstructed a previously affected scan from its existing candidate ledger: all seven findings were recovered, coverage was complete, seven SARIF results were generated, and all 11 sealed artifact hashes verified.

Copy link
Copy Markdown
Collaborator Author

@codex review Please review exact head 68e2171, focusing on preserving valid findings when severity change conditions arrive as string arrays, retaining strict rejection for malformed entries, maintaining canonical schema compatibility, and preventing complete coverage from being downgraded.

@github-actions github-actions Bot added the bug Something isn't working label Jul 31, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 68e21719bf

ℹ️ 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".

Comment on lines +845 to +847
and all(
isinstance(condition, str) and condition.strip()
for condition in change_conditions

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject invalid Unicode before joining conditions

When an array entry is a JSON string containing an unpaired UTF-16 surrogate such as "\ud800", this predicate accepts it because it is a nonblank str, and the list is normalized instead of rejected. The later document-wide _require_safe_json_value call then raises outside the per-finding recovery handler, causing the entire scan completion to fail rather than discarding the malformed finding and marking coverage partial. Validate each condition with _require_safe_json_string while still inside this finding's try block.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector

Copy link
Copy Markdown

Security review completed. No security issues were found in this pull request.

Reviewed commit: 68e21719bf

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant