Conversation
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. |
224e8d0 to
55a6339
Compare
Whole-stack QA against mainCompleted the combined-stack comparison with GPT-6 Astra at ultra before starting individual PR reviews. Baseline: No stack-only functional regression was observed in the covered Linux contracts. This is not an all-platform pass.
The one SDK failure is the unchanged malformed-Git-config case in Additional checks through built helpers covered 209 paired core scenarios and 155 paired rank/pool/scope scenarios, including a ten-stage shard-to-scope workflow and independently checked artifact bytes, modes, ordering and receipt hashes. All 155 rank scenarios match. Core comparisons match ordinary behavior; the two acceptance differences are 4,301-digit integer inputs rejected by Python's default conversion limit. Diagnostic wording also differs. I do not see a reason to copy those incidental Python limits or tracebacks into Node. Representative core and complete rank compositions also pass through the installed package. Integration and platform gapThe current stack tip does not contain #836's latest follow-up commits. For this QA I merged the current #843 and #836 heads onto main in an isolated checkout. Before claiming the combined stack is ready across platforms, restack to include those fixes and run CI on that result. The original pinned PR heads have successful Windows/macOS, native-target and PowerShell CI evidence. Those runs test their original trees; none tests this combined integration tree. Local execution here establishes Linux behavior only. Source-matched CI native payloads passed the distribution compatibility checks; ordinary workstation-built payloads exceeded the GLIBC distribution floor on both revisions. The stack adds a net 1,581 production lines and 2,078 test/proof lines. The individual reviews are examining concrete deletions and simpler implementations, particularly the JSON/schema/path adapters, while preserving real artifact and filesystem contracts. No implementation changes or formal review disposition are included in this QA comment. |
|
Reviewed The scope-binding migration is useful and correctly connected to the SDK workflow. I found no introduced correctness or security defect. One nonblocking reduction removes 65 formatted lines by using compact JSON for unsealed documents and diagnostics, while preserving exact numeric values and final sealing behavior. Verification beyond the complete-stack comparison with main:
Pre-seal JSON presentation and diagnostics intentionally change in the proposed simplification. The shared argument-parser alternative is attributed to #839, where that code originated, with its CLI compatibility changes stated separately. Local OS execution was Linux. Exact-head CI supplies separate Windows, PowerShell, macOS and native-runtime evidence. The alternatives need platform CI when applied. Restack the later PRs onto the latest #836 and rerun that CI for the combined stack before merging. |
|
Addressed both the PR review and the whole-stack QA. The shared serializer remains because the remaining migration uses its canonical representation for sealed artifacts and binding manifests. The stack is now restacked: this tip contains every current parent, including the latest Windows file-operation fixes and the shard/pool cleanup. All inline discussions across the eight PRs have a fix or a rationale. Updated head: This closes the combined-stack platform gap: All 14 inline threads across the eight PRs now have a fix or an explicit rationale and are resolved; all nine general review comments are addressed. The PRs remain open for review approval. |
Summary
Move repository scope binding to the bundled TypeScript helper and use it in generated scan instructions.
Changes
Testing
ba12023: both full SDK runs passed 2,656 tests with 50 skips and zero failures (seeds 12345 and 4201856736).Risk and rollout
Stacked on ranking pools. Invocation becomes
launch_codex_security_mcp[.cmd] --helper bind-repo-scopeswith the existing required scopes-file, manifest, and coverage options. The combined tip includes all current parent fixes. Actual Windows/macOS execution is checked by hosted CI.Public disclosure review