Conversation
Critical review with GPT-6 Astra at ultraReviewed The migration is worthwhile: candidate normalization and its MCP caller no longer need Python. No new correctness defect was found in the covered behavior. I left two nonblocking simplification comments: a 69-line reduction in numeric JSON parsing and a 12-line reduction in input ordering. #838 later shares the parser, so net savings across the full stack depend on that separate consumer review. The disposable variants preserve the tested previously accepted artifact bytes and candidate IDs. Their deliberate differences are accepting integral spellings such as Verification: 65 focused Bun tests passed with seven Windows-only skips; both affected MCP test files, nine portable-checker tests, and build/type/source checks passed. The 137-assertion baseline/head/variant harness passed under Node 22.22, then I independently replayed it under Node 22.13, 24.0, 26.0 and Bun 1.3.14: 137 checks passed in each run. All new execution was on Linux. Exact-head hosted Windows/macOS CI and the separate synthetic Windows consumer checks were inspected; the combined-tree platform gap remains documented in the shared QA record. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ 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. |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
|
Addressed both simplification comments: candidate parsing now uses JSON.parse and input paths use the direct deterministic sort. Canonical artifacts, IDs, line bounds, path checks, deduplication, and atomic replacement remain covered. Integral line spellings and malformed-input diagnostic differences are documented in the updated PR description. Updated head: |
![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: 2f3b519f7c
ℹ️ 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".
| signal, | ||
| ); | ||
| const findings = await parseImportedFindings( | ||
| source.toString("utf8"), |
There was a problem hiding this comment.
When a JSON or CSV import contains invalid UTF-8 bytes, Buffer.toString("utf8") silently replaces them with U+FFFD before validation. Such an import can therefore succeed with mutated titles, paths, identities, or other finding data, while the retained source artifact still contains different raw bytes. Decode with a fatal UTF-8 decoder and reject malformed input instead of importing corrupted findings.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
| if (directory !== undefined) | ||
| await rm(directory, { recursive: true, force: true }); |
There was a problem hiding this comment.
When feedback with log attachments has already been accepted but removing the temporary attachment directory fails—for example because a Windows scanner briefly retains a file handle—this finally block rejects the completed operation and reports the upload as failed. A user retry can then submit duplicate feedback even though only optional log cleanup failed; handle or report the cleanup error without replacing the successful upload result.
AGENTS.md reference: AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
This branch has not been deployed
2 participants
Summary
Replace Python candidate normalization with the bundled Node helper and update its MCP caller. Preserve canonical candidate bytes and IDs, accepted command arguments, raw path handling, and atomic output replacement.
Changes
Testing
Risk and rollout
This PR depends on #836. Canonical IDs, Unicode ordering, raw Unix paths, Windows native file handling, and atomic replacement remain compatibility requirements. The plugin helper invocation changes from
normalize_candidates.pytolaunch_codex_security_mcp[.cmd] --helper normalize-candidates; accepted normalization arguments and defaults are preserved. The SDK CLI is unchanged. Later PRs in the migration stack will need to incorporate this updated parent.Public disclosure review