feat: persist scan lifecycle and model metadata - #264
Conversation
|
@codex review Please review the exact current head |
|
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". |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ 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". |
|
Security 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. |
|
@codex review Please review the exact updated head |
|
Security 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. |
![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
Require context before closing false positives
The updated app bundle enables the Close button for false_positive with an empty context and no longer labels that context as required, but require_close_reason still rejects the resulting request with Explain why this finding is a false positive. Selecting False positive and immediately confirming therefore always produces an error instead of closing the finding. Restore the required-state and false-positive-specific prompt in the bundled app.
ℹ️ 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".
| warnings.append(warning) | ||
| manifest, findings, _ = _write_prepared_scan_finalization(prepared) | ||
| except ContractError as exc: | ||
| fail_scan( |
There was a problem hiding this comment.
When complete-scan or prepare-scan-completion encounters a repairable malformed draft artifact, this call permanently changes the scan from running to failed before returning the validation error. A failed scan cannot be completed later, so the agent cannot correct the manifest/findings/coverage and retry; this also contradicts the bundled MCP instructions that completion failure alone must not mark the durable scan failed. Preserve the running state on ContractError and reserve fail_scan for an explicit terminal failure.
Useful? React with 👍 / 👎.
| model=args.model, | ||
| reasoning_effort=args.reasoning_effort, |
There was a problem hiding this comment.
For the normal app-backed setup flow, the bundled MCP app invokes start_codex_security_scan with only sessionId, while the server handler ignores request metadata and forwards only its optional input fields. Consequently both values passed here are always None, and scans started through the setup UI still store null model and reasoning_effort even though the originating model turn provides them. Retain the metadata from the opening/waiting turn or read it in the start handler so this primary scan path is included.
Useful? React with 👍 / 👎.
| """ | ||
| UPDATE scans | ||
| SET model = COALESCE(?, model), reasoning_effort = COALESCE(?, reasoning_effort) | ||
| WHERE id = ? |
There was a problem hiding this comment.
When an idempotent start_codex_security_deep_scan call rejoins an already completed, failed, or canceled scan, require_owned_scan and the continuation check do not require the parent scan to be running, and this unrestricted update executes before the existing run is returned. A later retry from a turn using different model settings therefore rewrites the historical scan's model and reasoning_effort even though no scan work runs. Restrict this update to running scans so terminal execution metadata remains immutable.
Useful? React with 👍 / 👎.
|
@codex review Please review the exact updated head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dfdb63ee0
ℹ️ 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".
| findings = ( | ||
| row | ||
| for row in _active_findings(connection, read_coverage) | ||
| for row in _indexed_findings(connection) |
There was a problem hiding this comment.
When a later completed scan covers a finding's location but no longer reports that finding, _indexed_findings still yields the latest historical occurrence because it only ranks stored occurrences and never examines subsequent scan coverage. Consequently list-global-findings continues presenting the finding as open, and list-repositories also keeps it in openFindingsCount; the removed coverage-aware filtering previously handled this exact resolution case. Restore that filtering or persist an equivalent resolved state when scans complete.
Useful? React with 👍 / 👎.
| "findingCount": row["finding_count"], | ||
| "handoffStatus": row["handoff_status"], | ||
| "mode": row["mode"], | ||
| "model": row["model"], |
There was a problem hiding this comment.
For scans started through the public TypeScript SDK or CLI, api.ts invokes register-cli-scan, whose insert_running_scan call still supplies neither model nor reasoning_effort; this path also never uses the MCP progress endpoint that can backfill them. Although the saved recipe already contains the effective model and model_reasoning_effort, every such scan therefore exposes null in these newly added history fields. Pass the configured values during registration or extract them from the validated recipe.
Useful? React with 👍 / 👎.
|
Security 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. |
3 participants
Summary
Stack
main.Verification