[BorisTyshkevich]

What this does

Adds .github/workflows/pr-gate.yml — this repository's first PR-time verification
gate. Workflow name PR gate, single job Required PR gate, triggered on every
pull request (unfiltered) and every push to main, running exactly the five commands
issue #23 specifies:

go build ./...
go vet ./...
go test -race ./...
go test -tags phase5release ./internal/securitytest -count=1
bash integration/clickhouse/tests/lib-tests.sh

Until now the repo had no PR-time CI at all: the two existing workflows only build and
push images on push to main. This gate is a hard prerequisite for #33 (the ADR #32 LDAP
parser replacement), whose security-relevant changes must not depend on a coordinator
running local commands by hand.

Part of #23.

Design decisions

One job, five sequential steps — not multiple jobs plus an aggregator. This yields
exactly one merge-relevant check name and no needs graph. The decisive reason: a job
skipped by a job-level condition reports success, so an aggregator is a
silent-waiver shape rather than a fail-closed one.

Required PR gate is an externally consumed interface. It is the exact check-run
context branch protection on main will require, so renaming the job means migrating
branch protection in the same change. That is now machine-enforced, not just documented.

Minimal authority over untrusted PR code. pull_request (never
pull_request_target), permissions: contents: read, persist-credentials: false,
cache: false, no secrets, no registry login, nothing published. The explicit
permissions: block is load-bearing: this repository's default_workflow_permissions
is currently write.

Action pins. Both actions are pinned to full commit SHAs, each annotated with the
exact release tag it resolves to (actions/checkout@3d3c42e5… = v7.0.1,
actions/setup-go@b7ad1dad… = v7.0.0; both verified against the GitHub API), and both
are at or above the first release declaring the node24 runtime. Review pass 1
caught that the original pins (checkout@v4.4.0, setup-go@v5.6.0) both declare
using: node20, which currently runs only because the runners re-execute node20 actions
on Node 24 as a compatibility shim — withdrawn when Node 20 leaves the runner images on
2026-09-23. A brand-new mandatory gate would have broken repo-wide about three weeks
after landing. TestPRGateContract_ActionPinVersionCommentsAreAtOrAboveTheNode24Floor enforces a floor
on each pin's trailing version comment — deliberately named and worded for what it can
actually prove, since a commit SHA cannot be resolved to a version or a declared runtime
without network access a unit test must not have. Immutability itself is proven
separately and completely by TestPRGateContract_ExternalActionsArePinnedToFullCommitSHA. The two existing publisher workflows are deliberately left on floating tags —
separate hardening, out of scope here.

Trigger activity types are explicit, and edited is load-bearing. A bare
pull_request: defaults to opened/synchronize/reopened only. A base-branch retarget
arrives as edited and keeps the same head SHA, and the check-run is recorded against
that head SHA — so a PR proven green against another base could be retargeted onto
protected main and carry its green Required PR gate across without the new merge
result ever being tested. types: [opened, synchronize, reopened, edited] closes that,
and the contract test now requires exactly that list.

No path filters, no allowlists. Future tests added under internal/securitytest
(including the planned TestDependencyContract_ProductionClosureHasNoGeneralLDAP) enroll
in the gate with no workflow edit.

internal/securitytest/pr_gate_contract_test.go

The plan's invariant map originally left three invariants — "all five commands control
one merge result", "no skipped aggregator can report misleading success", and
"required-check name cannot silently drift" — enforced only by human static review. In a
repository that already keeps machine-checked contracts in internal/securitytest, that
was the weakest link, so this PR converts them into an executable contract over the
workflow YAML: trigger shape, absence of pull_request_target/path filters/if/needs/
continue-on-error, exactly one job, the literal job name, the five commands exactly once
each in order, and full-SHA action pinning.

Untagged, matching convention: release_gate_test.go is the only phase5release-tagged
file in that package. So it runs in plain go test ./... and in this gate's own
go test -race ./... step.

Two deliberate design constraints:

• Every expectation is a literal in the test source. A test that reads the job name
  out of the file under test and compares it to itself detects nothing.
• Action pins are asserted for immutability, not exact value (known identity +
  ^[0-9a-f]{40}$). Hardcoding today's SHAs would turn a legitimate action upgrade into
  unrelated test churn for no security gain.

Known limitation, stated plainly

This gate is not tamper-resistant against an authorized pull-request author. Because
the trigger is pull_request (correctly — not pull_request_target), a PR runs its own
copy of the workflow. A PR that keeps the job name but neuters the steps, or adds a
job-level if: false, produces a green required check that verified nothing. The
non-obvious part is that a skipped job satisfies a required status context, so reading
the steps during review does not catch it. The new contract test does not close this
either — it lives in the same PR-controlled tree and runs only because that same workflow
runs it.

So the honest claim is: this enforces process for trusted maintainers and catches ordinary
regressions. Closing the deliberate case would require an organization-level
required-workflow ruleset sourced from outside the repository being modified — org-owner
authority, a different enforcement architecture, and out of scope for #23. The full
reasoning and the decision to accept this is recorded in the issue's ship log.

Documentation reconciliation

Shipping this gate made several committed statements false, so they are fixed in the same
change (per CLAUDE.md's working discipline):

• CLAUDE.md — new repo-map rows; the gate described in Build & test; the
  Working discipline claim "There is no PR-time CI gate" replaced; the future-linter
  guidance now points at pr-gate.yml; Unlike the Go gate (was Like, which
  asserted the opposite of this commit); the integration/clickhouse/ row no longer
  implies nothing there is CI-gated.
• README.md — the new non-publishing workflow described alongside the two publishers.
• integration/clickhouse/README.md — tests/lib-tests.sh is in CI; only the Docker
  suite is manual.
• skills/ship/references/repo-footguns.md, skills/ship/SKILL.md — the stale "there is
  no PR-time CI to wait on" guidance replaced with head-SHA-keyed waiting on
  Required PR gate.

Two accuracy notes now recorded in CLAUDE.md: lib-tests.sh is Docker-daemon-free but
needs the docker CLI on PATH (lib/common.sh calls detect_compose_cmd at source
time and dies — reproduced locally), which makes the issue body's "needs only
bash/coreutils" imprecise; and the gate never compiles third_party/goldap /
third_party/ldapserver, which is where the parser #33 replaces currently lives.

Verification

All coordinator-run at head f743963, not taken from a self-report:

Check	Result
go build ./... / go vet ./...	exit 0
go test -race -count=1 ./...	exit 0
go test -tags phase5release ./internal/securitytest -count=1	exit 0
bash integration/clickhouse/tests/lib-tests.sh	exit 0
go vet -tags phase5release ./internal/securitytest	exit 0
bash helm/ch-oauth-ldap/test.sh	GATE RESULT: PASS (0 failures)
pinned actionlint v1.7.7 on pr-gate.yml	clean
workflow structure	independently re-derived by parsing the committed YAML

Sabotage cases for the new contract test — each must fail, and each failure message
must name the security consequence:

Mutation	Caught by
job renamed Required PR Gate	ExactlyOneJobNamedForBranchProtection
paths: ["**.go"] added under pull_request	TriggersAreUnfilteredPRAndMainPush + NoPathFiltersAnywhere
continue-on-error: true on the security step	JobIsUnconditionalAndFailsClosed
permissions: contents: write	TopLevelPermissionsAreReadOnly
actions/checkout@v4 (unpinned)	ExternalActionsArePinnedToFullCommitSHA
if: false on the job (the neutering shape above)	JobIsUnconditionalAndFailsClosed

All six failed as required; pr-gate.yml was restored byte-identical after each.

Hosted CI

Reviewed head: f743963. Required PR gate succeeded at that exact SHA in run
33283550959
(pull_request, all five steps: Build, Vet, Race tests, Phase 5 security tests, ClickHouse
shell library tests), as of 2026-08-30.

This section deliberately no longer tries to enumerate every run. It can't have a fixed
point: because the trigger now includes edited, editing this description starts another
gate run, which immediately makes any exhaustive table stale again. The complete
run-by-run ledger — every run, each keyed to its head SHA — lives in the
ship log,
whose edits do not retrigger the gate and which is current as of this head. Every wait in this PR was keyed to an exact head
SHA, never to gh run list ordering.

The two runs worth calling out from that ledger:

• c8538af — 33276319268
  ❌ at step 6. The gate found two real defects on its first ever run, neither
  reproducible on the darwin dev platform.
• e37e24d — 33277581488
  ❌ at step 7 only, with steps 4-6 passing: the deliberate phase5release red proof
  for Definition-of-Done item 11, reverted by a normal inverse commit so it stays in
  history.

What run 1 found. TestAdversarial_WriteDeadlineClosesStalledConnection… created
backpressure with a 20 MiB response and a 300ms WriteTimeout; on the runner all
~40 MiB was delivered, so the deadline never fired and the mechanism under test never
engaged (darwin buffers 128 KiB; the runner buffered everything). And a DATA RACE on
ldapserver.Logger turned out to be a consequence of that failure — the test
t.Fatalfs before its own teardown, leaking a live server into the next test's New(),
violating a constraint server.go already documents. Both fixes are test-only;
git diff origin/main..HEAD -- internal/ldap/server.go is empty. The stall now runs over
an unbuffered net.Pipe (deterministic on every host, no payload-size heuristic), and a
shared startServing helper gives seven server-start sites fail-safe teardown registered
before any assertion can abort.

DoD item 11 — the deliberate red proof (run 33277581488). Steps 4 Build, 5 Vet and
6 Race tests all passed; step 7 Phase 5 security tests failed; job Required PR gate failed. Two choices make it a proof rather than a coincidence: steps 4-6
passing means the red result is attributable to the security step alone, and the test
carried //go:build phase5release so it was invisible to go test -race ./... and fatal
only to the security command. Run 1's accidental failure could not serve this purpose —
it failed at step 6, which skipped steps 7-8, so the security step's wiring to the
required status had never been exercised. Removed by a normal inverse commit
(a11eac0), never a force-push, so the red run and its cause stay in this PR's history
as the audit trail.

Known limitation of the enforcement model, restated for the record

Branch protection is enabled after this merges and a successful push-event run is
observed on main: required context exactly Required PR gate, strict: true
(require branches up to date), enforce_admins: true, and no review requirement.
strict: true is load-bearing, not incidental — with loose required checks a PR green
against main = B0 can merge unchanged after another PR moves main to B1, since H
itself never gets a synchronize event, so a green gate would authorize an untested
B1+H merge result. That is the same fail-open class as the retarget case above, for the
far more common trigger. Read-back verifies all four fields plus the GitHub Actions app
binding, not just the context string. #33 stays blocked until all three hold: merged, green run on main,
and protection reading back the exact context. Merging this PR alone does not satisfy
that. And per the limitation above, the gate enforces process for trusted maintainers
rather than resisting deliberate tampering — closing that would need an organization-level
required-workflow ruleset sourced outside this repository.

🤖 Generated with Claude Code

https://claude.ai/code/session_011KxntXZk6BFoz3uT3wFtiS

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: a11eac0b21468a77ef909170f202ca71ba9bd509

Findings

1. Major — the new startServing fail-safe can still leak a server and recreate the races it was introduced to prevent.

   internal/ldap/protocol_test.go:startServing silently ignores both 15s timeout branches. If waiting for serveErr times out, it still calls srv.Stop() without the channel receive that this same helper documents as the load-bearing synchronization between Serve()'s unsynchronized Server.Listener write and Stop()'s read. If the subsequent srv.Stop() wait times out, the helper cancels and returns while that goroutine may still be live, allowing the next test to construct another server and potentially reproducing the cross-test ldapserver.Logger race from the first hosted run.

   There is also a concrete way for the first timeout path to be reached in the new test listener. internal/ldap/adversarial_test.go:singleConnListener says that after Close, every Accept returns an error, but Accept selects between a preloaded handout channel and the closed channel. If Close() happens before the first Accept(), both cases are ready and Go may return the connection after the listener is closed. I reproduced that exact Close-before-Accept sequence in isolation: 5,007 of 10,000 calls accepted the queued connection after Close returned.

   Fix: make singleConnListener deterministically honor Close-before-Accept (e.g. check closed state before consuming the handout, with appropriate synchronization), and make cleanup timeouts fail closed rather than silently return. In particular, do not call srv.Stop() until the Serve synchronization point has actually been observed, and do not let cleanup finish while the Stop goroutine is still unjoined. Mark the test failed on a bound breach, then preserve lifecycle ordering while completing teardown (the package-level test timeout can remain the ultimate hard bound).

2. Medium — the newly mandatory gate is born on already-deprecated Action runtimes.

   The exact-head hosted run is green, but its log warns that both pinned actions target Node 20 and are being forcibly run on Node 24. The pinned manifests confirm actions/checkout@11d596... and actions/setup-go@40f158... both declare node20. Current upstream releases already provide native Node 24 variants: actions/checkout v7.0.1 (3d3c42e5aac5ba805825da76410c181273ba90b1) and actions/setup-go v7.0.0 (b7ad1dad31e06c5925ef5d2fc7ad053ef454303e). For an ordinary optional workflow this could be follow-up work; for the status that will become mandatory branch protection, avoidable runtime deprecation is an eventual repo-wide merge-outage risk.

   Fix: verify the current Node-24 major releases against this workflow, pin their exact SHAs, keep human-readable release comments, and rerun the exact-head gate. This does not require touching the existing publisher workflows.

3. Minor — the canonical PR body no longer reflects the reviewed head or completed hosted proof.

   The PR is now at a11eac0..., but its Verification section still says the checks were coordinator-run at c8538af, and the deliberate hosted red run is still described in future tense. The red proof has in fact already happened (run 33277581488: build/vet/race passed, phase5 security failed, shell step skipped) and the restored head run 33277682239 is green through all five commands.

   Fix: refresh the PR description before merge so its audit trail names the current reviewed/restored head and the completed red/green run IDs rather than an obsolete intermediate state.

Focused checks

• Canonical PR head re-read immediately before this review: a11eac0b21468a77ef909170f202ca71ba9bd509; 12 changed files, mergeable.
• Run 33277682239 at this head reports one Required PR gate job and all five contractual commands succeeded; token permissions in the job log are contents: read plus GitHub's implicit metadata read.
• Run 33277581488 verifies the deliberate phase5release failure propagates to the single required job.
• I found no separate defect in the net.Pipe substitution itself: it still traverses the production Server.Serve/handler/write-deadline path while making backpressure deterministic. The blocking issue is the surrounding listener/cleanup lifecycle above.
• The workflow contract test's core expectations are source literals rather than values derived from the YAML under test; I found no self-reference defect in that mechanism.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: a11eac0b21468a77ef909170f202ca71ba9bd509

Reviewed head: 30c403056488fd67dd089d8ec8e9d37e8961bae1

Reassessment of pass 1

• Node 20 action pins: resolved in the actual workflow. The new pins are native-Node-24 releases (actions/checkout@v7.0.1, actions/setup-go@v7.0.0), and exact-head run 33278654950 completed all five contractual steps successfully with no old Node-20 compatibility warning.
• Stale PR body: resolved for the substantive audit trail. It now names 30c4030 and records the deliberate phase5release red proof as completed. The hosted-run table could name run 33278654950 explicitly, but that is polish, not a merge blocker.
• Shared LDAP teardown/listener finding: only partially resolved. The sequential Close(); Accept() defect is fixed, but the timeout fix still executes the unsafe path after reporting it.

Findings

1. Major — startServing still proceeds unsafely after either teardown timeout.

   internal/ldap/protocol_test.go:startServing now calls t.Errorf on the two 15s bound breaches, but Errorf marks the test failed and continues execution. Therefore a serveErr timeout is immediately followed by launching srv.Stop() even though the helper's own documented synchronization point was not observed — exactly the Stop-racing-Serve path the helper says is a data race. Likewise, if the Stop wait times out, the helper reports the error, cancels, and returns while the Stop goroutine can still be live; that preserves the cross-test contamination mechanism from pass 1.

   Calling t.Errorf from cleanup is fine; the problem is continuation after it. The fix should fail the test and preserve the lifecycle invariant: after the first bound breach, cancel if useful but do not call Stop() until serveErr has actually been received; after the Stop bound breach, cancel and then join the Stop goroutine before cleanup returns. Let the package/test-binary timeout be the ultimate hard bound if teardown is truly unrecoverable. That prevents a broken cleanup from advancing to the next test with live server state.

2. Medium — the new Node-24 floor test does not bind the claimed version/runtime to the immutable SHA.

   TestPRGateContract_ActionPinsAreAtOrAboveTheNode24Floor derives the action identity from uses: but derives the version only from uses.LineComment. An old Node-20 SHA can therefore pass both contract tests if its trailing comment is left or changed to a high version, e.g. the old checkout SHA 11d5960a326750d5838078e36cf38b85af677262 with # actions/checkout@v7.0.1: it is still a full 40-hex pin, and the floor test sees only v7.0.1. The regex also does not bind the comment's action name to the uses: identity.

   So the actual v7 upgrade is good, but this new test does not machine-enforce the runtime property it claims. For an offline test, the sound options are either (a) keep a small literal allowlist/map of approved action identity -> SHA -> version/runtime and accept intentional test churn on action upgrades, or (b) remove/rename this as comment-hygiene rather than runtime enforcement. Also, if retaining a semantic floor, actions/checkout v5.0.0 already declares using: node24; v6.0.0 is not the first Node-24 release. actions/setup-go v6.0.0 is still Node 20, so its v7 floor is correct.

3. Medium — the workflow contract still has two concrete false negatives against invariants it says it enforces.

   • TestPRGateContract_TopLevelPermissionsAreReadOnly checks only the top-level permissions: map. GitHub also permits jobs.<job_id>.permissions; adding permissions: {contents: write} to required-pr-gate would give that job a different token permission set while this test still passes. If the invariant is "PR code receives only contents: read", reject job-level permissions entirely (or validate it as exactly read-only too).
   • TestPRGateContract_FiveCommandsRunExactlyOnceInOrder uses strings.Count over each run: scalar. A step such as run: echo 'go build ./...' or a comment containing the command satisfies the substring check without executing the command. The current workflow uses one-line commands, so the simpler and stronger contract is to require the normalized run: values to equal the five literal commands in order.

   These do not make the current pr-gate.yml unsafe; the current workflow is correctly read-only and executes the five real commands. They do mean the new accidental-drift detector is weaker than its stated security contract.

Focused checks

• Canonical head was re-read immediately before this review: 30c403056488fd67dd089d8ec8e9d37e8961bae1, 8 commits / 12 changed files, mergeable.
• The delta from a11eac0... is exactly two commits: 8e6e1be... and 30c4030....
• Exact-head run 33278654950 is green through Build, Vet, Race tests, Phase 5 security tests, and ClickHouse shell-library tests; its job token is contents: read plus GitHub's implicit metadata read.
• The upgraded action SHAs resolve to the stated v7 tags, and both pinned action.yml files declare node24.
• The extra closure pre-check in singleConnListener.Accept fixes the pass-1 sequential Close-before-Accept case. I found no current-call-site regression from that extra select.
• t.Errorf in a t.Cleanup-registered closure is valid; the defect is that Errorf deliberately continues, so the unsafe cleanup state transition still occurs.
• main remains unprotected and Implement ADR #32: replace general LDAP stack with ClickHouse compatibility profile #33 remains blocked, which is consistent with ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23's post-merge rollout: DoD 10 and 12 are not satisfied merely by this PR being green. The deliberate-red DoD item is already evidenced in PR history.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed head: 30c403056488fd67dd089d8ec8e9d37e8961bae1

Reviewed head: e82c74988930d8c73fdfb0d2ef128a031dca7cfe

Reassessment of earlier findings

• Native Node 24 action pins: resolved in the actual workflow. The current checkout/setup-go SHAs both declare node24, and exact-head run 33279680085 is green through all five required commands.
• Job-level permission override: resolved. The contract now checks a job-level permissions: block independently and accepts only absence or the same exact {contents: read} mapping.
• Substring command matching: resolved. run: scripts are reduced and exact-matched against the five literal commands; the old echo 'go build ./...' bypass is closed.
• Action floor boundaries: the literal boundaries are now factually correct (checkout v5.0.0, setup-go v6.2.0), but the SHA-binding defect from pass 2 remains.
• Shared teardown: improved, but the pass-2 lifecycle finding is not fully closed; timeout paths can still return while the prior server lifecycle is live.

Findings

1. Major — a PR base-branch retarget can reuse a green Required PR gate that tested the old merge result.

   .github/workflows/pr-gate.yml uses bare pull_request:. GitHub's default pull_request activity types are only opened, synchronize, and reopened; changing the PR base is an edited activity. Therefore retargeting a PR does not start this workflow unless the head also changes.

   This matters for the exact enforcement model here. The hosted run checks out and tests the synthetic merge ref (df02cfe... for this head/base), but GitHub records Required PR gate on the PR head SHA e82c749...; there is no check run on that synthetic merge SHA. Required-status evaluation falls back to the latest head commit when the test merge commit has no status. A PR can therefore be tested against base A, retain the same head SHA, be retargeted to protected main, and still carry the successful required check even though the merge result against main was never exercised.

   Fix: explicitly include the base-change activity: pull_request.types: [opened, synchronize, reopened, edited], and update the contract test to require exactly that activity set while still forbidding path/base-branch filters. edited also fires for title/body edits, but this gate is deliberately cheap; an extra full run is safer than introducing a conditional job/step that can create another skipped-success path.

2. Major — runShutdownSequence can still return from cleanup with live old-server state.

   The first timeout now correctly avoids the immediate unsynchronized Stop() call, but it reports, cancels, and returns while the Serve goroutine is still live. cancel() only cancels the LDAP handler root context; it is not a substitute for observing Serve terminate. The vendored accept loop can still touch package-global ldapserver.Logger when it eventually returns, while the next test's New() unconditionally reassigns that same global.

   The second timeout is even more direct: after Stop() exceeds the bound, the helper reports, cancels, and returns while the Stop goroutine may still be live. Stop() itself touches ldapserver.Logger after closing the listener and while/after waiting on clients. The new unit test demonstrates this lifecycle explicitly: after runShutdownSequence returns from the simulated Stop timeout, it has to close(block) itself to let the leaked Stop goroutine finish before the test exits. A focused standalone reproduction of the exact helper behavior likewise confirms the helper returns while the fake Stop is still blocked.

   This will already make the test red, so it is not a gate-green bypass; the problem is cross-test isolation and race attribution. It preserves the mechanism that originally made one failed test surface as a ldapserver.Logger race in the next test.

   Fix: a timeout may mark the test failed, but cleanup must not allow the next test to start until this server lifecycle has actually quiesced. On Serve timeout, cancel if useful and then wait for the real serveErr synchronization before any Stop() call. On Stop timeout, cancel and then join the stopped channel before returning. If teardown is irrecoverably wedged, let the package/test-binary timeout be the ultimate fail-stop rather than continuing with a second Server in the same process.

3. Medium — the Node-24 floor test still does not bind its version/runtime claim to the immutable SHA.

   The new regex correctly binds the trailing comment's identity to the uses: identity, and both corrected version floors are accurate. But checkActionNode24Floor still receives only (identity, lineComment); it never receives the SHA. Thus this still passes the two contract checks:

   uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # actions/checkout@v7.0.1

   That SHA is the old Node-20 pin. It is a valid 40-hex immutable ref, and the floor test sees a same-identity v7.0.1 comment, so it passes. The current workflow itself is safe — its actual v7 SHAs are Node 24 — but the test still does not machine-enforce the runtime property its name/documentation claim.

   Fix: simplest is to remove/rename this extra floor test as comment hygiene; issue ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23 requires immutable pins, not an offline runtime oracle. If machine enforcement is worth keeping, use a small literal identity -> approved SHA -> known version/runtime mapping and accept intentional test churn when these security-sensitive actions are upgraded. Also reconcile the file-header comment, which still states the obsolete checkout >= v6.0.0 / setup-go >= v7.0.0 floors while the actual map now correctly uses v5.0.0 / v6.2.0.

4. Minor — the canonical PR description is stale again.

   It still says verification was run at 30c4030 and its hosted-CI table labels 30c4030 as (current), while the canonical head is now e82c749... and run 33279680085 is the current exact-head green proof. Refresh that audit trail before merge.

Focused checks

• Canonical head re-read immediately before this comment: e82c74988930d8c73fdfb0d2ef128a031dca7cfe; previous reviewed head 30c403056488fd67dd089d8ec8e9d37e8961bae1; the delta is exactly two commits and only internal/ldap/protocol_test.go plus internal/securitytest/pr_gate_contract_test.go changed.
• Exact-head hosted run 33279680085 has one Required PR gate job and Build, Vet, Race tests, Phase 5 security tests, and ClickHouse shell-library tests all succeeded.
• I independently rechecked the runtime boundaries: checkout v4.3.1 is Node 20 and v5.0.0 is Node 24; setup-go v6.1.0 is Node 20 and v6.2.0 is Node 24. The current v7 pinned SHAs both declare Node 24.
• I found no regression in the net.Pipe write-deadline substitution or the fixed singleConnListener Close-before-Accept behavior.
• main remains unprotected, which is still consistent with the stated post-merge rollout for DoD 10; it is not the reason for this REVISE verdict.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: 67e1fb7cc6bda3c3ca93372da35072bfb5e7730d

Findings

1. Major — the strict: true rollout correction is not actually present in the canonical ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23 rollout record.

   The re-certification handoff and the new 67e1fb7 commit both say the post-cap branch-protection finding was accepted and recorded, with protection to be configured strict/up-to-date. But the current canonical ship-log comment on issue ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23 still says:

   enforce_admins: true, strict/up-to-date false, no review-count requirement.

   Its current GitHub updated_at also predates this correction. This is not merely stale prose: that ship log is the recorded post-merge procedure. If followed literally, it recreates the base-advancement fail-open from the architecture review. GitHub's strict mode is the control that requires a PR to be tested with the latest protected-branch code; loose mode explicitly permits merging without bringing the branch up to date.

   The post-merge bootstrap ordering itself is acceptable: the workflow has to land and emit its stable check before classic protection can require it, and Implement ADR #32: replace general LDAP stack with ClickHouse compatibility profile #33 remains blocked until enforcement is active. The problem is that the authoritative rollout instruction still specifies the unsafe setting.

   Fix: before certification/merge, correct the canonical ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23 rollout record to require strict: true, enforce_admins: true, the exact Required PR gate context, and the expected GitHub Actions App source. After merge, observe the push-event run on the resulting main SHA, apply protection, and read back all of those fields before treating Implement ADR #32: replace general LDAP stack with ClickHouse compatibility profile #33 as unblocked.

2. Medium — the Node-24 check is now honestly scoped in prose, but its public/test surface still overclaims what it proves.

   The latest code explicitly states the right limitation: TestPRGateContract_ActionPinsAreAtOrAboveTheNode24Floor checks the trailing human-readable version comment, not the pinned SHA's actual release/runtime, and a Node-20 SHA paired with a plausible same-action above-floor comment still passes. I agree with the simpler resolution: issue ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23 requires immutable action pins, and an approved SHA/runtime table is not necessary just to satisfy that contract.

   However, the test is still named ActionPinsAreAtOrAboveTheNode24Floor, its diagnostics describe the comment-derived version as the action being "pinned to v...", and the canonical PR body still says the test "now enforces the floor." Those surfaces continue to communicate SHA/runtime enforcement that the implementation deliberately does not provide.

   Fix: rename the test/helper/diagnostics around what is actually checked (for example, ActionVersionCommentsAreAtOrAboveNode24Floor, with messages saying the version comment is below the floor), and update the PR description to call this comment hygiene rather than runtime enforcement. Keep the independent 40-hex immutability check as the actual pin contract.

3. Minor — the canonical PR verification/audit section is stale at 22b9aad.

   The PR is currently at 67e1fb7cc6bda3c3ca93372da35072bfb5e7730d, and hosted run 33281026583 is green through Build, Vet, Race tests, Phase 5 security tests, and ClickHouse shell-library tests. The PR body still says coordinator verification was at 22b9aad and labels that older head as current.

   Fix: refresh the PR description before merge so the audit trail names the exact reviewed head and current hosted run.

Focused checks

• Re-read canonical head immediately before posting: 67e1fb7cc6bda3c3ca93372da35072bfb5e7730d; PR is open and mergeable, 12 changed files.
• Delta from 22b9aadce354a6c94b7cd5487a5b7f6a1993187b is exactly one commit touching only internal/ldap/protocol_test.go and internal/securitytest/pr_gate_contract_test.go.
• The teardown correction is sound: startServing now preserves the load-bearing ln.Close() -> <-serveErr -> srv.Stop() -> cancel() ordering under sync.Once/t.Cleanup, and no timeout branch can release the next test with old server state still live. If shutdown truly wedges, the test process/job timeout is the fail-stop boundary.
• Exact-head run 33281026583 has one Required PR gate job and all five contractual commands succeeded; token permissions in the log are contents: read plus GitHub's implicit metadata read.
• I inspected the complete current 12-file PR, not just the latest two-file delta, and found no additional confirmed workflow fail-open beyond the still-uncorrected strict:false rollout instruction.
• A local clone/package rerun was not feasible in this sandbox because outbound DNS could not resolve github.com; the canonical GitHub fetch path and exact-head hosted Linux gate were used instead.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: 67e1fb7cc6bda3c3ca93372da35072bfb5e7730d

Reviewed head: 56b253279de89bfe9cf583c7673558d94891ab7c

Reassessment of pass 1

• Major — canonical rollout still specified strict: false: resolved. Issue ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23's canonical ship log now contains an explicit authoritative rollout table requiring exact context Required PR gate (app github-actions), strict: true, enforce_admins: true, and no review-count requirement, with read-back instructions. main is still unprotected today, but that is the documented bootstrap state: Implement ADR #32: replace general LDAP stack with ClickHouse compatibility profile #33 remains blocked until this PR is merged, a green push-event gate is observed on main, and protection is applied/read back. I do not consider the post-merge ordering itself a PR defect.
• Medium — Node-24 check name/diagnostic overclaimed SHA/runtime enforcement: resolved. The only code delta since 67e1fb7 renames the test/helper around version comments and changes the below-floor diagnostic from "pinned to v..." to "version comment claims v...". The PR body now states the same limitation and keeps immutable-SHA validation separate. I do not require an identity→SHA→runtime table for ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23: the issue requires immutable action pins; the comment-floor check is optional hygiene as long as it is honestly scoped. The documented floors are factually consistent with the action manifests (checkout v5.0.0 is node24; setup-go v6.1.0 is node20 and v6.2.0 is node24).
• Minor — stale PR verification/audit head: resolved. The canonical PR body now records verification at 56b2532 and marks that head current.

Finding

1. Minor, non-blocking — one stale sentence survived the Node-24 rename.

   In internal/securitytest/pr_gate_contract_test.go, the scope-limit comment still says:

   SCOPE LIMIT, stated plainly because the test's name promises more than it can deliver

   That was true before the rename; it is no longer true for TestPRGateContract_ActionPinVersionCommentsAreAtOrAboveTheNode24Floor, whose name now accurately describes a comment check. Reword this to state the scope limit without claiming the current test name overpromises. Separately, the PR body's heading Hosted CI — five runs now precedes more than five historical runs; that is similarly harmless audit-text drift.

   Neither item weakens enforcement or changes the ship decision.

Focused checks

• Delta 67e1fb7cc6bda3c3ca93372da35072bfb5e7730d...56b253279de89bfe9cf583c7673558d94891ab7c is exactly one commit and one file: internal/securitytest/pr_gate_contract_test.go, 30 insertions / 30 deletions. The changes are naming/diagnostic wording; gate logic is unchanged.
• I reused the complete pass-1 review of the 12-file PR and rechecked the current complete scope. The other 11 changed files are unchanged since the previous reviewed head; I found no new regression in them.
• The workflow still has one unconditional Required PR gate job, read-only permissions, immutable action SHAs, explicit edited PR activity, and the five contractual commands sequentially.
• Exact-head runs 33281621345 and the later same-head rerun 33281711455 both completed successfully; each Required PR gate job passed Build, Vet, Race tests, Phase 5 security tests, and ClickHouse shell-library tests.
• main is still unprotected pre-merge, and Implement ADR #32: replace general LDAP stack with ClickHouse compatibility profile #33 is still open with the explicit precondition that ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23/equivalent be active and enforced before production parser replacement code merges. Those are post-merge rollout obligations, not evidence that this PR's gate can currently fail open.
• I found no remaining confirmed merge-gate fail-open in the updated PR.

VERDICT: SHIP

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed head: 56b253279de89bfe9cf583c7673558d94891ab7c

Reviewed head: 8a0dc6ae1f4e13216f5f41be2db975085dcc7b21

Reassessment of pass 2

• Minor — stale Node-24 scope sentence: resolved. The only code delta since 56b2532 rewrites the scope-limit comment to state directly that the check verifies the version COMMENT, not the SHA/runtime. The current test/helper/diagnostic names remain accurately scoped to version-comment hygiene.
• Minor — stale Hosted CI — five runs heading: resolved. The canonical PR body now uses Hosted CI — every run, each keyed to its exact head and records coordinator verification at 8a0dc6a.
• All earlier substantive findings remain resolved: the authoritative ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23 rollout requires strict: true; startServing preserves ordered fail-stop teardown; the immutable-action check is separate from comment-floor hygiene; the workflow includes edited; permissions and five commands remain fail-closed and exact.

Findings

1. Minor, non-blocking — the current-head hosted-run row is already stale again.

   The PR body currently ends its hosted-run table with:

   (pending) | 8a0dc6a | comment/heading wording — current head

   but exact-head runs 33282032417 and 33282035937 have both completed successfully. This does not affect enforcement, but if the table is intended as the audit record, replace the pending row with the completed run(s) before merge.

2. Minor, non-blocking — the contract-test header uses past tense for a future Node-20 removal date.

   internal/securitytest/pr_gate_contract_test.go says GitHub Actions runners stopped shipping Node 20 on 2026-09-23. As of August 30, 2026, GitHub's current changelog schedules runner Node-20 removal for September 23, 2026; the date is correct, but stopped is premature. Use future tense such as will remove Node 20 on 2026-09-23 / will stop including Node 20....

   This is documentation only and does not change the Node-24 pins or the ship decision.

Focused checks

• Delta 56b253279de89bfe9cf583c7673558d94891ab7c...8a0dc6ae1f4e13216f5f41be2db975085dcc7b21 is exactly one commit, one file, 6 insertions / 6 deletions; it changes comments only.
• I reused the full prior review of all 12 changed files and rechecked the current complete PR scope. The workflow itself is unchanged: one unconditional Required PR gate job, contents: read, SHA-pinned actions, explicit opened/synchronize/reopened/edited, and exactly the five contractual commands in order.
• Exact-head runs 33282032417 and 33282035937 both completed successfully; the Required PR gate job passed Build, Vet, Race tests, Phase 5 security tests, and ClickHouse shell-library tests.
• The canonical ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23 ship log still requires exact context Required PR gate (app github-actions), strict: true, enforce_admins: true, no review-count requirement, and read-back of those settings/app binding.
• main remains unprotected pre-merge and Implement ADR #32: replace general LDAP stack with ClickHouse compatibility profile #33 remains open with ci: add required PR-time Go/race/security gates; keep Docker ClickHouse matrix separate #23/equivalent enforcement as a precondition. That is the documented bootstrap state, not a new fail-open: remaining work is merge -> successful push run on the resulting main SHA -> apply protection -> read it back -> only then unblock Implement ADR #32: replace general LDAP stack with ClickHouse compatibility profile #33.
• I found no remaining confirmed merge-gate fail-open or code regression at this head.

VERDICT: SHIP