feat(cli): add interactive finding review, patching, and optional PRs - #461
feat(cli): add interactive finding review, patching, and optional PRs#461ianw-oai wants to merge 4 commits into
Conversation
6ab52c3 to
93e9967
Compare
mldangelo-oai
left a comment
There was a problem hiding this comment.
Non-blocking QA notes on draft 93e99673. The saved-finding patch flow, selection and cancellation UI, and successful local Git publication path worked with synthetic fixtures. The full local suite passed 1,316 tests with 11 skipped. Git used a local remote, and PR creation used a test double, so this was not a live GitHub PR test.
I'd split the work at the patch result: return one structured result for saved findings, literal issues, and Linear input, then let a separate publication step consume it. The saved-finding-only --create-pr path does not yet join up with #475 and #477.
Two concrete Windows follow-ups: use platform-resolved paths in the new tests, and add *.tsx text eol=lf to .gitattributes. The new TUI file passes formatting with LF and fails with CRLF. The current Windows checks still fail. I left the recovery and simplification suggestions inline.
| await run("git", ["push", "--set-upstream", "origin", branch]); | ||
| const url = await run("gh", [ | ||
| "pr", | ||
| "create", | ||
| "--head", |
There was a problem hiding this comment.
Non-blocking: if PR creation fails after the push, the branch and commit already exist. A retry then hits git switch -c with the same branch name and fails. I reproduced this with a local remote and a failing PR-service test double. Could publication retain the branch and commit, then resume at the missing push or PR step without rerunning the model? A regression where push succeeds, PR creation fails, and a retry succeeds would cover it.
| const findingPatchSchema = z.object({ | ||
| occurrenceId: z.string(), | ||
| status: z.enum(["verified", "no_change", "blocked", "failed"]), | ||
| files: z.array(z.string()), | ||
| verification: z.string().optional(), | ||
| reason: z.string().optional(), | ||
| }); |
There was a problem hiding this comment.
Non-blocking simplification: could literal issues, Linear issues, and saved findings share this result boundary? One typed result with the source ID, task ID, outcome, changed files, and verification would let JSON output and publication use the same code. I'd adapt the skill's outcome vocabulary once here instead of keeping a saved-finding-only parser and separate success rules. That would also distinguish a completed model turn that reports blocked from an issue that was fixed.
| expect(invocation[invocation.indexOf("--cd") + 1]).toBe( | ||
| "/current/other/repository", | ||
| ); |
There was a problem hiding this comment.
Non-blocking: these expectations use POSIX absolute paths, while the implementation resolves paths for the host platform. The Windows jobs fail on this family of assertions. Could we define the fixture roots once with node:path's resolve and join, then reuse them in setup and expectations? The later CODEX_HOME, patch-editor repository, and saved-repository assertions need the same treatment.
| function detailLines( | ||
| value: unknown, | ||
| indent = "", | ||
| evidence?: ReadonlyMap<string, FindingCodeEvidence>, | ||
| ): string[] { |
There was a problem hiding this comment.
Non-blocking scope suggestion: could we reuse the existing finding presentation for expanded details and keep this UI focused on selection, severity, and short instructions? It worked at 80x24, but one synthetic finding produced 70 detail lines. Reusing the detail view and handing longer conversations to the saved task would reduce the rendering and keyboard state this feature has to maintain.
93e9967 to
648adac
Compare
Summary
After a security scan finishes, choose which findings to fix, generate and check the fixes, and optionally open a GitHub pull request. The same flow also works without prompts and with findings from earlier scans.
Built on #477. Each selected finding runs in its own saved Codex task, which appears in the Codex desktop app.
Changes
Review findings after a scan
npx @openai/codex-security scan .The scan prints its usual findings, then asks whether to review and patch them. Scans with no findings do not ask.
Choose a severity, select individual findings, inspect the surrounding code, and give the patch instructions for each finding:
After you press
Enter, each selected finding gets its own Codex task. Tasks run one at a time in the same checkout, and a failed finding does not stop the remaining tasks.Patch automatically
Patch findings from an earlier scan
Open a pull request after verified fixes
npx @openai/codex-security scan . --patch --patch-severity high --create-pr npx @openai/codex-security patch --scan latest --severity high --create-prOnly verified fixes are committed. Existing staged changes are left alone.
Testing
pnpm run typespnpm run formatpnpm run buildpnpm run check:package ../../dist/openai-codex-security-0.1.12.tgzRisk and rollout
Patching and pull request creation are optional. Incomplete scans are not patched. Saved Codex tasks can contain finding details. Code shown in the review stays inside the scanned repository, and existing authentication and failure behavior are preserved.
Public disclosure review