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. |
![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: 4eb900ce78
ℹ️ 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 ($branch -cne $ExpectedBranch) { throw 'Audit branch mismatch.' } | ||
| if ($head -cne $ExpectedHead.ToLowerInvariant()) { throw 'HEAD mismatch.' } | ||
| if ($main -cne $expectedHeadLower) { throw 'Local main mismatch.' } | ||
| if ($originMain -cne $expectedHeadLower) { throw 'origin/main mismatch.' } | ||
| if ($main -cne $ExpectedMain.ToLowerInvariant()) { throw 'main/base mismatch.' } |
There was a problem hiding this comment.
When this handoff is run from a current post-PR clone, the checkout is on main and local main points at the merged audit commit or later, rather than the old 5374c1c... base. These checks therefore reject the repository before validation can start; bind HEAD, local main, and origin/main to the externally supplied post-merge SHA instead of requiring the obsolete audit branch/base.
Useful? React with 👍 / 👎.
| $branch = Invoke-GitText @('branch', '--show-current') | ||
| $head = Invoke-GitText @('rev-parse', 'HEAD') | ||
| $main = Invoke-GitText @('rev-parse', 'main') |
There was a problem hiding this comment.
If an operator obtains ExpectedHead from a mistaken or malicious clone, the preflight now checks only local refs and never verifies either the origin URL or origin/main. A repository can retain the expected historical base and manifest-listed blobs while altering unlisted code, yet still produce a successful baseline; restore validation of the official remote and require its main ref to match the supplied commit.
Useful? React with 👍 / 👎.
| $target = Get-ConfinedManifestFile $script:RepoPath $manifestPath.Replace('/', [System.IO.Path]::DirectorySeparatorChar) | ||
| $repositoryPath = $manifestPath.Replace('\', '/') | ||
| $actual = Get-GitBlobSha256 $repositoryPath | ||
| if ($actual -cne $expected) { throw 'Operational checksum mismatch.' } | ||
| $actual = (Get-FileHash -LiteralPath $target -Algorithm SHA256).Hash.ToLowerInvariant() | ||
| if ($actual -cne $expected) { throw 'Checkpoint checksum mismatch.' } |
There was a problem hiding this comment.
On the intended native Windows host with core.autocrlf=true, tracked text files are clean but contain CRLF working-tree bytes, while manifest 57 records the LF Git-blob hashes. Get-FileHash consequently reports a checksum mismatch for a valid fresh clone; compute hashes from git cat-file blob HEAD:<path> so checkout newline conversion does not invalidate the preflight.
Useful? React with 👍 / 👎.
| $isAdmin = $principal.IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator) | ||
| $trackedSymlinkEntries = @((Invoke-GitText @('ls-files', '-s')) -split "`n" | Where-Object { $_ -match '^120000 ' }) | ||
| $trackedSymlinkCount = $trackedSymlinkEntries.Count | ||
| $trackedSymlinkCount = ((Invoke-GitText @('ls-files', '-s')) -split "`n" | Where-Object { $_ -match '^120000 ' }).Count |
There was a problem hiding this comment.
This repository has no tracked symlinks, so the filtered pipeline produces no object; with Set-StrictMode -Version Latest, accessing .Count on that null result throws before the preflight can emit JSON. Wrap the pipeline in @(...) and take the resulting array's count.
Useful? React with 👍 / 👎.
| diagnostic=diagnose() | ||
| self.assertEqual(diagnostic.backend_type,"native") | ||
| self.assertRegex(diagnostic.artifact_sha256 or "",r"^[0-9a-f]{64}$") | ||
| self.assertEqual(diagnostic.artifact_sha256,"602c643b17d7d863e801d4a4eca12711b2724698d0e2d822711fd724cf9b74c1") |
There was a problem hiding this comment.
When the documented Windows 3.10/3.12/3.14 matrix runs, the native secp256k1 DLL cannot have the same digest as this particular native artifact, so the suite fails even though diagnose() correctly reports an unapproved native backend with signing disabled. Assert that the detected artifact has a valid SHA-256 value rather than pinning one platform/build-specific binary.
Useful? React with 👍 / 👎.
| self.path.parent.mkdir(parents=True,exist_ok=True) | ||
| try: | ||
| with closing(self._connect()) as db: | ||
| with self._connect() as db: |
There was a problem hiding this comment.
A sqlite3.Connection context manager commits or rolls back but does not close the connection. On Windows, especially when an exception traceback retains the frame, these reverted usages can keep database file handles open and make run-owned temporary-directory cleanup fail; restore contextlib.closing or explicit close() handling for this store and the similarly changed coordinator stores.
Useful? React with 👍 / 👎.
|
C9.1 was validated at |
1 participant
This PR reverts the unintended C9.1 commits that were merged after the local-only test scope was clarified.\n\nReverted commits:\n- 43d3b18 Fix C9.1 preflight scalar counts\n- f066234 Fix C9.1 preflight worktree count\n- beef26d Fix C9.1 Windows main handoff\n\nThe resulting tree matches 12ed2fa.