Rectify incomplete merge-gate evidence - #321
Closed
lamemustafa wants to merge 53 commits into
Closed
lamemustafa wants to merge 53 commits into
lamemustafa wants to merge 53 commits into
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: b9ad301a1c
ℹ️ 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".
lamemustafa
force-pushed
the
tooling/merge-gate
branch
2 times, most recently
from
September 12, 2026 07:50
e0e6dc3 to
5bf8320
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bf83200bb
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33bdc32182
ℹ️ 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".
lamemustafa
force-pushed
the
tooling/merge-gate
branch
from
September 12, 2026 08:27
ba1ee75 to
f9b6df9
Compare
The gate for "may this PR be merged" has lived in one session's head, and it has been got wrong from both directions in a single day. On 2026-09-12 four PRs were merged before their reviews arrived — #312 by three seconds — leaving eleven findings, four of them P1, on code already in master. The gate added to stop that then deadlocked a clean PR, because a Codex pass with no findings submits no review object at all, only a summary row and a thumbs-up. Later the same day two separate sessions reported a PR "clean, zero open threads" from a query issued seconds before its review posted, and a third reported clean without noticing master had moved. One rule underneath all of it: **a review is only evidence about the commit it names.** "No findings yet" and "not looked yet" are indistinguishable unless you read which commit was looked at — so the script reads the commit in the summary table, not the review list and not the comment's timestamp, because that comment is edited in place and its `created_at` means nothing. Six checks, each carrying the incident that motivates it: 1. mergeable against its base; 2. based on master — `ci.yml` fires on `pull_request` into master only, so a stacked PR runs no CI at all and its green tick measures nothing; 3. every check concluded and none failing; 4. a Codex review naming *this* head, completed rather than running; 5. zero unresolved threads, paginated, since `required_conversation_resolution` makes this the gate rather than a courtesy — and a `first:100` page once hid 19 open threads; 6. a privacy scan of the **whole merge diff**, not the author's own commits: a real transaction reference sat in a comment on public master through two PRs because each author scanned only what they wrote. The scan was wrong twice while being written, both times in the direction that reports clean: - `XXXXX1234X` is a fabricated PAN and `X` is an uppercase letter, so it matched the PAN shape. A gate that cries wolf gets ignored, which is worse than no gate, so obvious placeholders are excluded — but by an explicit list, never by widening the shape, which would start excusing real values. - the first placeholder pattern used a backreference, which is not ERE. grep errored, returned no matches, and the scan reported clean. That is the same shape as a control reporting zero because its branch never fired, so the pattern is now probed against a string it must match before being trusted. - UUID tails are twelve hex characters and often all digits; the canonical `550e8400-…-446655440000` tripped it. UUIDs are stripped before scanning. Verified in both directions rather than assumed: a diff carrying a real-shaped GSTIN, mobile number and account number is flagged; one carrying only placeholders and a UUID is not. Run against three live PRs it blocks each for a different, correct reason — a running review, unresolved threads, and a non-master base. Usage: `scripts/merge-gate.sh <pr> [owner/name]`; exit 0 may merge, 1 must not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… catch
Eight findings on the first revision, plus two more found by testing it rather
than reading it. A script whose whole purpose is rigour had ten holes, and the
two that matter most were found by running it against the very case it exists
for.
**The scan could not see the leak that motivated it.** The pattern was
`\b[0-9]{11,18}\b`. The reference it was written to catch is written
`HDF CH12345678901` — glued to letters — and `\b` does not match between `H`
and `1`. Verified directly: the old pattern returns nothing on that string.
The gate would have passed the exact commit it was built to stop. The word
boundaries are gone.
**It scanned removals as well as additions,** so it blocked the PR that
*deletes* leaked data while passing the one that adds it. Added lines only now.
Then the eight from review, each confirmed against the tool it names:
- `BEHIND` was not blocked. `mergeable` can read `MERGEABLE` while
`mergeStateStatus` is `BEHIND`, meaning the head never saw the current base —
so its CI and its review describe a tree that no longer exists.
- `gh pr checks` buckets are `pass|fail|pending|skipping|cancel`. The regex
looked for `cancelled`, so a cancelled check counted as neither failing nor
pending and read as success. Now the JSON buckets are read instead of the
text columns, which a check name containing spaces mis-splits anyway.
- A review reported `Failed` was treated exactly like `Completed`. A failed run
means nothing looked at the code — the original bug inverted. Only
`Completed` passes.
- `--repo owner/name` was documented and not parsed: `REPO` became the literal
`--repo`. Proper option parsing, and `OWNER/NAME` is validated.
- On a transient GraphQL failure the thread query fell back to a hardcoded
repository with the same PR number. A same-numbered PR elsewhere with no
open threads would have read as a clean result. The fallback is gone.
- The issue-comments query was unpaginated; the Codex summary is an ordinary
comment and the default page is 30.
- Nothing bound the merge to the reviewed commit. The script now prints the
merge command carrying `--match-head-commit <sha>`.
- Draft and non-OPEN states were not checked.
Dropping `\b` made hex digests visible — a sha256 in a lockfile contains long
digit runs by chance — so digests and UUIDs are stripped before scanning.
Both are machine-generated and neither can carry a client identifier; the
placeholder list was deliberately *not* loosened, because loosening the shape
starts excusing real values.
Verified in both directions rather than assumed. On a 4,799-line real diff:
zero false positives. On constructed input: the glued-to-letters reference, a
real-shaped GSTIN and a real-shaped mobile number are all flagged, while a PR
that only removes a leak is not. Every exit path exercised — malformed
`--repo`, missing value, unknown option, no arguments all return 2; a stale
head returns 1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Running the gate across the whole open queue surfaced two false positives, both
worth fixing rather than tolerating — a gate that cries wolf gets ignored,
which is the failure mode this one is least able to afford.
`00000000005551` in a constructed test page is a padded MICR fixture, not an
account number. Excused by `^0{6,}[0-9]{1,5}$`, kept deliberately narrow: a
real account number can begin with a zero or two, so only a run of six or more
leading zeros — plainly synthetic — is excused. Verified that three
real-shaped account numbers with one and two leading zeros are still flagged.
The other was this script flagging itself. A comment named the canonical RFC
example UUID with its middle elided, so the UUID stripper could not match it
while its digits still read as an identifier. The comment no longer carries
example digits at all: a literal in a comment is a literal in the diff, and
this scan reads its own file like any other — which is exactly how the
reference that motivated the gate reached public master in the first place.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round two on the gate: seven findings, two of them holes a determined actor could walk through, and one the gate found in its own author the moment it ran. **The review row was spoofable.** The summary was located by a marker string in a comment body — and a marker is just text. Any PR participant could post an ordinary comment carrying that marker and a `Completed` row for the current SHA, and the gate would have recorded a review that never happened. It now requires the comment's author to be `chatgpt-codex-connector[bot]` with a `Bot` account type, checked alongside the marker. **A binary file was an invisible pass.** `gh pr diff` emits `Binary files … differ` instead of content, so a screenshot or PDF of a client statement produced a scan with nothing to find and a clean verdict — the exact shape of a control reporting zero because it cannot see, which this repository has written down twice. Binary changes now block and are named for inspection by hand. **Exemptions are reported rather than silent.** Stripping UUIDs and hex digests keeps the false-positive rate low enough that the gate gets read at all, but a blanket exemption nobody can see is how a real value gets erased. The count of exempted lines is now printed. Four more, each confirmed: - `mergeStateStatus=BLOCKED` fell into a wildcard that printed `ok`, reading as approval for a state GitHub is refusing. It now prints a note, and an unrecognised state blocks rather than being guessed at. - `reviewThreads` blocked permanently once a PR passed 100 threads, since `hasNextPage` stays true however many are resolved. A PR accumulates threads by being reviewed carefully, so the rule punished exactly the PRs it should trust. It paginates now. - `gh pr checks` exits nonzero both when a check fails and when the query fails, so empty stdout from a broken query was reported as "no checks" — a statement about the PR rather than about the request. The two are now distinguished by whether anything reached stderr. - AGENTS.md:23 requires every PR to link a completed line in `review-checklist.md`, and the gate did not check the repository's own stated pre-merge rule. It does now — and immediately blocked this PR, whose description did not carry one. Each fix exercised rather than assumed: the author filter admits only the Bot account; the binary matcher fires on a binary patch and not on a text one; the thread loop terminates and counts across pages; the checklist rule blocks a PR without the link. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round three: seven findings, three of them P1, and one is a third blind spot in the privacy scan — the third distinct way the scan could be handed data it would not look at. **`++` at the start of a line hid a value from the scan.** The filter dropped `^+++` to skip the unified-diff file header, but an added line whose own content begins with `++` produces exactly that prefix. `++ customer ABCDE1234F` yielded no scannable text at all. The header is now identified structurally — `+++ b/<path>` or `+++ /dev/null`, never the bare prefix — so content reaches the scan and headers still do not. **A seven-hex prefix is 28 bits and can be ground deliberately.** The review row was matched by substring, so an author could push a commit sharing the reviewed prefix and have a stale `Completed` row vouch for code nobody read. Codex publishes only seven characters, so the comparison cannot be strengthened — but a ground commit must be created *after* the review it impersonates, and that is checkable. The row's own timestamp is now compared against the head commit's committer date, and a review that predates its head is refused. The match is also anchored to the backtick cell, since an unanchored substring matched the row's timestamp and URL too — neither of which is a claim about a commit. **Identifiers were matched uppercase-only.** A GSTIN or PAN written in lower or mixed case is the same identifier, and prose is exactly where it would be written that way. Matching is case-insensitive now, with the placeholder list applied to the uppercased form so `xxxxx1234x` stays excused. Four more: - A binary **deletion** emits `Binary files a/x and /dev/null differ` and was counted like an addition, so the gate blocked the PR removing a leaked screenshot. Same inversion as scanning removed lines, in a different rule. - `--match-head-commit` validates only the head, so the base can be changed after the check without moving the head. The printed command now re-reads the base and compares it before merging. - A failed comments query with empty stdout was reported as "no Codex review summary at all" — "I could not ask" stated as "there is no review", which is the exact substitution this script exists to prevent. - `--repo=` with an empty value was accepted. Both of the first two fixes were wrong on first writing and caught by their own controls: the header filter still swallowed `+++ customer …` until it matched `b/` and `/dev/null` explicitly, and the case-insensitive match used `-O` for `-o`. Every rule is now exercised in both directions — content reaches the scan while real headers do not, a lowercase PAN is flagged while a lowercase placeholder is not, a binary add blocks while a binary delete does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three sessions hit the same defect independently in one day: a pinned file's bytes moved and the compatibility surface was not resealed. Once by editing, once by running `cargo fmt` *after* resealing, and once by rebasing — where the rebase takes the base's manifest and the author touches nothing. That spread of causes is the point. The rule people had written down was "the reseal is the last step before `git add`", and each of us broke it while believing we were following it, because each filed it under the *situation* we had just been in rather than under the actual invariant: **any operation that can change the bytes of a pinned file — an edit, a formatter, a merge, a rebase — invalidates the seal, and the reseal runs after the last of them.** Nothing in the local loop re-reads pins before a commit, so CI's gate is the only thing that notices, and every instance therefore reaches a reviewer instead of its author. That makes it a class, not a set of mistakes, and a class is worth closing here rather than writing down again. The check needs no checkout: read the 211 pinned paths from the manifest at the PR head, intersect with the PR's changed files, and require the manifest to have moved if any of them did. It is deliberately weaker than CI's gate and says so: it proves the reseal was *performed*, not that the hashes are *right*. Only the real gate proves that. But every instance observed was a reseal that never ran at all, so this catches the whole observed failure while costing one API call. Verified in three directions rather than two: a pinned file changed without the manifest blocks; the same change with the manifest passes; a PR touching the manifest alone has nothing to reseal and passes. Against live PRs, #314 reports one pinned file with the manifest moved alongside it, and #320 reports nothing pinned. Credit where due — this was suggested by the lane on #288, which had just been bitten by the `cargo fmt` variant, on the grounds that closing the class beats closing the instances. It was right. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… literals A comment explaining the `++` blind spot used a realistic PAN as its example, and the newly case-insensitive matcher flagged it. Correctly: the scan reads this file like any other, and **a literal that illustrates a leak pattern is the pattern**. Third time a comment here has tripped the check — a UUID, a partial UUID, now a PAN. Each time it was the check working. The rule is now written in the comment so the next person adding an example reads it first: describe the shape, never spell it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lamemustafa
force-pushed
the
tooling/merge-gate
branch
from
September 12, 2026 08:37
f9b6df9 to
c19255a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c19255ab89
ℹ️ 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".
added 5 commits
September 12, 2026 16:33
lamemustafa
changed the title
The gate for "may this PR be merged" has lived in one session's head, and it has been got wrong from both directions in a single day. On 2026-09-12 four PRs were merged before their reviews arrived — #312 by three seconds — leaving eleven findings, four of them P1, on code already in master. The gate added to stop that then deadlocked a clean PR, because a Codex pass with no findings submits no review object at all, only a summary row and a thumbs-up. Later the same day two separate sessions reported a PR "clean, zero open threads" from a query issued seconds before its review posted, and a third reported clean without noticing master had moved. One rule underneath all of it: **a review is only evidence about the commit it names.** "No findings yet" and "not looked yet" are indistinguishable unless you read which commit was looked at — so the script reads the commit in the summary table, not the review list and not the comment's timestamp, because that comment is edited in place and its `created_at` means nothing. Six checks, each carrying the incident that motivates it: 1. mergeable against its base; 2. based on master — `ci.yml` fires on `pull_request` into master only, so a stacked PR runs no CI at all and its green tick measures nothing; 3. every check concluded and none failing; 4. a Codex review naming *this* head, completed rather than running; 5. zero unresolved threads, paginated, since `required_conversation_resolution` makes this the gate rather than a courtesy — and a `first:100` page once hid 19 open threads; 6. a privacy scan of the **whole merge diff**, not the author's own commits: a real transaction reference sat in a comment on public master through two PRs because each author scanned only what they wrote. The scan was wrong twice while being written, both times in the direction that reports clean: - `XXXXX1234X` is a fabricated PAN and `X` is an uppercase letter, so it matched the PAN shape. A gate that cries wolf gets ignored, which is worse than no gate, so obvious placeholders are excluded — but by an explicit list, never by widening the shape, which would start excusing real values. - the first placeholder pattern used a backreference, which is not ERE. grep errored, returned no matches, and the scan reported clean. That is the same shape as a control reporting zero because its branch never fired, so the pattern is now probed against a string it must match before being trusted. - UUID tails are twelve hex characters and often all digits; the canonical `550e8400-…-446655440000` tripped it. UUIDs are stripped before scanning. Verified in both directions rather than assumed: a diff carrying a real-shaped GSTIN, mobile number and account number is flagged; one carrying only placeholders and a UUID is not. Run against three live PRs it blocks each for a different, correct reason — a running review, unresolved threads, and a non-master base. Usage: `scripts/merge-gate.sh <pr> [owner/name]`; exit 0 may merge, 1 must not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… catch
Eight findings on the first revision, plus two more found by testing it rather
than reading it. A script whose whole purpose is rigour had ten holes, and the
two that matter most were found by running it against the very case it exists
for.
**The scan could not see the leak that motivated it.** The pattern was
`\b[0-9]{11,18}\b`. The reference it was written to catch is written
`HDF CH12345678901` — glued to letters — and `\b` does not match between `H`
and `1`. Verified directly: the old pattern returns nothing on that string.
The gate would have passed the exact commit it was built to stop. The word
boundaries are gone.
**It scanned removals as well as additions,** so it blocked the PR that
*deletes* leaked data while passing the one that adds it. Added lines only now.
Then the eight from review, each confirmed against the tool it names:
- `BEHIND` was not blocked. `mergeable` can read `MERGEABLE` while
`mergeStateStatus` is `BEHIND`, meaning the head never saw the current base —
so its CI and its review describe a tree that no longer exists.
- `gh pr checks` buckets are `pass|fail|pending|skipping|cancel`. The regex
looked for `cancelled`, so a cancelled check counted as neither failing nor
pending and read as success. Now the JSON buckets are read instead of the
text columns, which a check name containing spaces mis-splits anyway.
- A review reported `Failed` was treated exactly like `Completed`. A failed run
means nothing looked at the code — the original bug inverted. Only
`Completed` passes.
- `--repo owner/name` was documented and not parsed: `REPO` became the literal
`--repo`. Proper option parsing, and `OWNER/NAME` is validated.
- On a transient GraphQL failure the thread query fell back to a hardcoded
repository with the same PR number. A same-numbered PR elsewhere with no
open threads would have read as a clean result. The fallback is gone.
- The issue-comments query was unpaginated; the Codex summary is an ordinary
comment and the default page is 30.
- Nothing bound the merge to the reviewed commit. The script now prints the
merge command carrying `--match-head-commit <sha>`.
- Draft and non-OPEN states were not checked.
Dropping `\b` made hex digests visible — a sha256 in a lockfile contains long
digit runs by chance — so digests and UUIDs are stripped before scanning.
Both are machine-generated and neither can carry a client identifier; the
placeholder list was deliberately *not* loosened, because loosening the shape
starts excusing real values.
Verified in both directions rather than assumed. On a 4,799-line real diff:
zero false positives. On constructed input: the glued-to-letters reference, a
real-shaped GSTIN and a real-shaped mobile number are all flagged, while a PR
that only removes a leak is not. Every exit path exercised — malformed
`--repo`, missing value, unknown option, no arguments all return 2; a stale
head returns 1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Running the gate across the whole open queue surfaced two false positives, both
worth fixing rather than tolerating — a gate that cries wolf gets ignored,
which is the failure mode this one is least able to afford.
`00000000005551` in a constructed test page is a padded MICR fixture, not an
account number. Excused by `^0{6,}[0-9]{1,5}$`, kept deliberately narrow: a
real account number can begin with a zero or two, so only a run of six or more
leading zeros — plainly synthetic — is excused. Verified that three
real-shaped account numbers with one and two leading zeros are still flagged.
The other was this script flagging itself. A comment named the canonical RFC
example UUID with its middle elided, so the UUID stripper could not match it
while its digits still read as an identifier. The comment no longer carries
example digits at all: a literal in a comment is a literal in the diff, and
this scan reads its own file like any other — which is exactly how the
reference that motivated the gate reached public master in the first place.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round two on the gate: seven findings, two of them holes a determined actor could walk through, and one the gate found in its own author the moment it ran. **The review row was spoofable.** The summary was located by a marker string in a comment body — and a marker is just text. Any PR participant could post an ordinary comment carrying that marker and a `Completed` row for the current SHA, and the gate would have recorded a review that never happened. It now requires the comment's author to be `chatgpt-codex-connector[bot]` with a `Bot` account type, checked alongside the marker. **A binary file was an invisible pass.** `gh pr diff` emits `Binary files … differ` instead of content, so a screenshot or PDF of a client statement produced a scan with nothing to find and a clean verdict — the exact shape of a control reporting zero because it cannot see, which this repository has written down twice. Binary changes now block and are named for inspection by hand. **Exemptions are reported rather than silent.** Stripping UUIDs and hex digests keeps the false-positive rate low enough that the gate gets read at all, but a blanket exemption nobody can see is how a real value gets erased. The count of exempted lines is now printed. Four more, each confirmed: - `mergeStateStatus=BLOCKED` fell into a wildcard that printed `ok`, reading as approval for a state GitHub is refusing. It now prints a note, and an unrecognised state blocks rather than being guessed at. - `reviewThreads` blocked permanently once a PR passed 100 threads, since `hasNextPage` stays true however many are resolved. A PR accumulates threads by being reviewed carefully, so the rule punished exactly the PRs it should trust. It paginates now. - `gh pr checks` exits nonzero both when a check fails and when the query fails, so empty stdout from a broken query was reported as "no checks" — a statement about the PR rather than about the request. The two are now distinguished by whether anything reached stderr. - AGENTS.md:23 requires every PR to link a completed line in `review-checklist.md`, and the gate did not check the repository's own stated pre-merge rule. It does now — and immediately blocked this PR, whose description did not carry one. Each fix exercised rather than assumed: the author filter admits only the Bot account; the binary matcher fires on a binary patch and not on a text one; the thread loop terminates and counts across pages; the checklist rule blocks a PR without the link. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round three: seven findings, three of them P1, and one is a third blind spot in the privacy scan — the third distinct way the scan could be handed data it would not look at. **`++` at the start of a line hid a value from the scan.** The filter dropped `^+++` to skip the unified-diff file header, but an added line whose own content begins with `++` produces exactly that prefix. `++ customer ABCDE1234F` yielded no scannable text at all. The header is now identified structurally — `+++ b/<path>` or `+++ /dev/null`, never the bare prefix — so content reaches the scan and headers still do not. **A seven-hex prefix is 28 bits and can be ground deliberately.** The review row was matched by substring, so an author could push a commit sharing the reviewed prefix and have a stale `Completed` row vouch for code nobody read. Codex publishes only seven characters, so the comparison cannot be strengthened — but a ground commit must be created *after* the review it impersonates, and that is checkable. The row's own timestamp is now compared against the head commit's committer date, and a review that predates its head is refused. The match is also anchored to the backtick cell, since an unanchored substring matched the row's timestamp and URL too — neither of which is a claim about a commit. **Identifiers were matched uppercase-only.** A GSTIN or PAN written in lower or mixed case is the same identifier, and prose is exactly where it would be written that way. Matching is case-insensitive now, with the placeholder list applied to the uppercased form so `xxxxx1234x` stays excused. Four more: - A binary **deletion** emits `Binary files a/x and /dev/null differ` and was counted like an addition, so the gate blocked the PR removing a leaked screenshot. Same inversion as scanning removed lines, in a different rule. - `--match-head-commit` validates only the head, so the base can be changed after the check without moving the head. The printed command now re-reads the base and compares it before merging. - A failed comments query with empty stdout was reported as "no Codex review summary at all" — "I could not ask" stated as "there is no review", which is the exact substitution this script exists to prevent. - `--repo=` with an empty value was accepted. Both of the first two fixes were wrong on first writing and caught by their own controls: the header filter still swallowed `+++ customer …` until it matched `b/` and `/dev/null` explicitly, and the case-insensitive match used `-O` for `-o`. Every rule is now exercised in both directions — content reaches the scan while real headers do not, a lowercase PAN is flagged while a lowercase placeholder is not, a binary add blocks while a binary delete does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three sessions hit the same defect independently in one day: a pinned file's bytes moved and the compatibility surface was not resealed. Once by editing, once by running `cargo fmt` *after* resealing, and once by rebasing — where the rebase takes the base's manifest and the author touches nothing. That spread of causes is the point. The rule people had written down was "the reseal is the last step before `git add`", and each of us broke it while believing we were following it, because each filed it under the *situation* we had just been in rather than under the actual invariant: **any operation that can change the bytes of a pinned file — an edit, a formatter, a merge, a rebase — invalidates the seal, and the reseal runs after the last of them.** Nothing in the local loop re-reads pins before a commit, so CI's gate is the only thing that notices, and every instance therefore reaches a reviewer instead of its author. That makes it a class, not a set of mistakes, and a class is worth closing here rather than writing down again. The check needs no checkout: read the 211 pinned paths from the manifest at the PR head, intersect with the PR's changed files, and require the manifest to have moved if any of them did. It is deliberately weaker than CI's gate and says so: it proves the reseal was *performed*, not that the hashes are *right*. Only the real gate proves that. But every instance observed was a reseal that never ran at all, so this catches the whole observed failure while costing one API call. Verified in three directions rather than two: a pinned file changed without the manifest blocks; the same change with the manifest passes; a PR touching the manifest alone has nothing to reseal and passes. Against live PRs, #314 reports one pinned file with the manifest moved alongside it, and #320 reports nothing pinned. Credit where due — this was suggested by the lane on #288, which had just been bitten by the `cargo fmt` variant, on the grounds that closing the class beats closing the instances. It was right. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… literals A comment explaining the `++` blind spot used a realistic PAN as its example, and the newly case-insensitive matcher flagged it. Correctly: the scan reads this file like any other, and **a literal that illustrates a leak pattern is the pattern**. Third time a comment here has tripped the check — a UUID, a partial UUID, now a PAN. Each time it was the check working. The rule is now written in the comment so the next person adding an example reads it first: describe the shape, never spell it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round four: five findings, three P1, and the first retracts last round's fix. **The timestamp defence was void.** To authenticate a review against a ground seven-hex prefix, the previous revision compared the review's time against the head commit's committer date. `GIT_COMMITTER_DATE` is set by whoever creates the commit: an author able to grind a prefix can also backdate it. The check read as coverage and provided none, which is worse than the gap it papered over. Replaced with something an author cannot set: **uniqueness.** A ground collision puts two commits sharing the prefix in the PR, so the PR's own commit list is counted and more than one match blocks. The residual is stated in the code rather than implied away: uniqueness within the PR does not exclude a collision created elsewhere and force-pushed as the sole commit. Closing that needs Codex to publish a full SHA. Until then this is a deterrent, not a proof, and it says so. **A phone number written normally was invisible.** `+91 98765 43210` scans as three short segments, under both the 10-digit and 11-digit thresholds. The first fix stripped every separator to make a projection — and invented eight findings on a clean diff, joining two adjacent dates into a sixteen-digit run and `CE_ADR_0016_E` into a PAN shape. A gate that cries wolf gets ignored, which costs more than this catches. Separators are now permitted only *inside* a phone-shaped run, and the result is re-checked as a ten-digit Indian mobile, so nothing unrelated is fused. Verified: five separator styles caught, and all three invented findings gone. **A filename is content.** Header lines are dropped from the scan, so a file whose basename carries an identifier passed with safe contents. Destination paths are added back as their own scannable text. Two more: an unreadable compatibility manifest was treated as an absent one and the pin check silently skipped — it now asks whether the path exists and fails closed if it does; and `gh pr view --json files` hard-codes `files(first: 100)`, so a PR touching more than 100 files returned a partial list and a pinned file outside the first page read as untouched. The paginated REST endpoint is used instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26baf27ed3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd0ab9ad52
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c0db06f85
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 830c0289bf
ℹ️ 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".
added 4 commits
September 13, 2026 04:54
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ea1235396
ℹ️ 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".
Owner
Author
|
The reviewed implementation is carried forward in #335 at The final preflight for this PR passes the protected-check, current-base, description and compatibility checks, but rejects synthetic identifier examples embedded in older commit messages. The replacement preserves the runtime gate, parser, workflow and compatibility artifacts exactly; its sole source difference assembles the same grouped-number test values from smaller pieces. The focused grouped-number control passes, and the privacy rule remains unchanged. This PR remains the original review and authorship record. It stays open until #335 has its own completed provider review, current CI, independent acceptance and final preflight. Any new finding on this head will still be assessed against the replacement. |
Owner
Author
|
Closing in favour of #335, which supersedes this PR. Both PRs rewrite the same six files ( This PR's review discussion and authorship record are preserved here; the 95 resolved threads on it are not being discarded, and the work they produced lives on in #335's branch. Closed as superseded, not rejected. |
lamemustafa
added a commit
that referenced
this pull request
Sep 15, 2026
* Rectify incomplete merge preflight evidence Carry forward the reviewed implementation from PR #321. The original PR preserves its review and authorship record. Assemble synthetic grouped-number probes without embedding complete identifier-shaped literals in source. * Rectify merge gate evidence controls * fix: reject structural-only policy summaries and reseal workflow * fix: fail closed on merge-gate review evidence * Harden merge-gate review evidence * Tighten platform evidence continuations * Bound platform evidence list continuations * Avoid literal home paths in gate fixtures * fix(gate): close review evidence gaps * fix(gate): harden platform and credential checks * test(gate): cover renamed sensitive paths * fix(gate): require affirmative platform evidence * test: avoid privacy scan fixture false positives * fix: require security review for bank import credentials * fix: require security review for cache token handling * fix: strengthen merge-gate security evidence * test: qualify workflow security fixtures * fix: extend merge gate credential and privacy coverage * fix: extend merge gate credential boundaries * rectify: modularize merge gate privacy policy * rectify: validate skipped CI jobs at final fence * rectify: review credential dependency lockfiles * rectify: harden privacy and security review gates Avoid static secret-like literals in the regression fixture so external secret scanning can evaluate the final PR range without suppressing the privacy-gate coverage. * rectify: classify quoted credential assignments * rectify: harden merge gate final fences * merge-gate: cut to only what runs as a real required CI check The merge-gate tooling was 3977 lines that gated nothing automatically (a manual CLI; master's ci.yml never invoked it). Review produced 20 unresolved threads and 27 tracking issues. Per the owner's decision, keep only what can run as a real required CI check and delete the rest. Kept: - Compatibility-surface validation (read_surface_paths() and callers). - Privacy/PII scan (merge_gate_privacy.py, merge_gate_diff.py in full, plus their input pipeline: commit author/committer metadata validation and the diff-coverage scan in merge-gate.sh). - A new, minimal (38-line) check that a review exists naming the current head SHA, closing #317: PRs merged 2-8 minutes after opening, where "zero unresolved threads" meant "review had not started". Treats "no review evidence for this head" as FAIL. Cut entirely: - PR/head/base identity binding, branch-protection context derivation, and the checks rollup. - PR body/review-checklist/P4/platform-evidence validation. - Skipped-CI-job allowlist and security-reviewer-comment validation. - scripts/merge_gate_fake_gh.py (a GitHub API test double that existed only to test the cut concerns). merge-gate.sh: 1587 -> 552 lines. merge-gate.test.py shrunk to cover only the kept surface, backed by a new, much smaller fake `gh` (merge_gate_test_gh.py) replacing the deleted one. Also fixes 7 verified live defects in the kept code (#346, #358, #360, #365, #366, #373, #377), each with a regression test in merge-gate.test.py verified to fail against the pre-fix code and pass after. docs/proposed-merge-gate-ci.md proposes the CI wiring to actually make this a required check (a new workflow file, not touching .github/ per this change's scope) and explains why the review-evidence check needs pull_request_review/issue_comment triggers, not just push/synchronize. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(merge-gate): lock in coverage for all 11 surviving PR #335 privacy findings Investigated all 11 unresolved review findings against the promoted-to- required scripts/merge_gate_privacy.py. All 11 turned out to already be fixed on fix/merge-gate-cut-336: each fix landed in an earlier commit (d16416d, 7a29762, c033dd4, 57799cb, f41d634) that is an ancestor of this branch's head, confirmed by direct behavioral testing against the scanner plus git ancestry checks. Add PrivacyScannerFindingsPR335, a direct-unit-test class against merge_gate_privacy.scan(), with one positive + one negative test per finding (22 tests total) so this behavior has explicit regression coverage -- no prior test exercised these specific sub-cases. Each was verified to fail when the specific historical defect described in its review thread is reintroduced, and pass once reverted back out (see task report for the per-finding before/after transcripts). All fixtures use fabricated values (fake UUIDs, .test-domain emails, placeholder credential strings, truncated non-decodable PEM bodies) -- no real PII or credentials. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: t <dev@example.invalid> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #328. Addresses #317's incomplete merge-evidence failure mode.
Functional summary
Add a read-only preflight that binds its result to the full current PR head and master tip. It requires the exact protected-context set, a completed current provider review, independent acceptance, complete review/file/commit inventories, description evidence and compatibility manifests. Known failures return MUST NOT MERGE; missing or malformed evidence returns INDETERMINATE. The command never changes GitHub state.
Check and combined-status pagination validate complete, unique, current-head records. Large context and review payloads travel through files, avoiding Linux argument limits; failed diagnostic computation cannot become success. Required skipped checks remain blocking, while deliberate optional skips are allowed. A summary-only provider receipt needs an explicit independent-review attestation naming the complete head.
The diff parser preserves payload using Git's LF delimiter, including Unicode separators and CRLF content. Independently quoted paths, textual destinations and rename metadata reconcile with REST file records. Added payload, paths, PR text, commit messages and standard Git identity fields enter the bounded privacy heuristic. Horizontal Unicode spaces normalize without joining distinct lines. Coverage and metadata-only diagnostics redact private path shapes and bound every example. Null linked GitHub identities remain valid; ordinary names and email addresses are not automatically classified as private data. Binary changes require human inspection, and gitlinks remain indeterminate pending provenance, license and NOTICE review.
The description must link an actual checklist item at the complete candidate SHA and provide a concrete validation command. Populated canonical template fields are accepted; empty labels, hidden HTML comments (including unclosed comments), bare placeholders and unrelated sibling fields are insufficient. Raw body bytes remain available to privacy scanning and final drift detection. Checklist anchors require a real delimiter after the line number. Security impact accepts explicit None/N/A/no impact responses while rejecting pending or TODO. Implementation additions, including shell, PowerShell and SQL code, require all three P4 answers. Native and platform-sensitive paths require affected-host evidence or a justified unaffected-host statement; workflow and database migration paths require rollback notes. Renames retain their prior path for conditional checks. DSC/credential changes additionally need a fetched independent security-focused reviewer record naming the full head.
Scope, reuse, and impact
Test or reproduction command
Candidate:
7ea1235396bb1ddf2cab0e9f5cdda079c65a2f17, including masterba1742cf0ec8f17c3f10d537a5bfa845739b946f.python3 scripts/merge-gate.test.py: all 118 controls passed at immutable2151a22923c1d222e0906d4691a595c9bcb97b40(490.465 seconds, exit 0). Input hashes and the clean head matched before and after execution. Covers malformed and paginated evidence, large Linux context inputs, Unicode/CRLF paths, visible/raw body handling, conditional policy fields, privacy diagnostics and gitlink holds.ba1742cf0ec8f17c3f10d537a5bfa845739b946fpreserved those three gate source files byte-for-byte. Compatibility tooling matches the accepted Rectify compatibility output mode preservation #334 tree; all 212 pins passed membership and actual-byte verification after regeneration.python3 scripts/merge-gate.test.py MergeGateControls.test_implementation_additions_need_all_three_p4_answers MergeGateControls.test_database_migration_paths_need_rollback_notes MergeGateControls.test_wrapped_canonical_migration_template_field_is_recognized. Full 118 execution is recorded at the earlier immutable head, not claimed for changed final bytes.bash -n scripts/merge-gate.sh, Python compilation andgit diff --checkpassed. Root reviewed the complete policy/parser changes; Luna independently accepted the immutable gate batch and final extension. Hosted provider review must separately complete on the exact published head.facf1cb292f0d9a16dfbf3b2aff7267fd5b0b98ahas the exact candidate tree98e191799f1ffe8235288239c5d7a0c53fc6667b, independently verified against both parents. Windows and macOS native checks and bundle smoke also pass; the compiler-cache retention skip is optional.Migration compatibility
No application schema, persisted data, Tally transport or credential change. The preflight supplements branch protection and required native/live evidence. Its field checks and privacy heuristic do not replace human assessment of test results, source completeness, licensing or private-data exposure. Independent acceptance also verifies whether production-regression policy applies, that the branch is dedicated to its fix, and that the required rectification label is present. AGENTS.md defines no branch-name prefix or authoritative metadata classifier; the gate documents this semantic responsibility instead of inventing one. This PR is dedicated to the merge-gate defect and carries
type:rectify.Rollback notes
Revert this PR to remove the advisory preflight and its Workflow consistency invocation. Branch protection remains in place. Run a fresh preflight immediately before a head-bound merge after relevant state changes.
Security impact
Read-only GitHub queries, numeric PR selection and fixed repository validation. Privacy diagnostics report counts instead of matched values. Credential and DSC paths are unchanged.