[BorisTyshkevich]

Part of #33 (execution tracker for ADR #32, Accepted) — Phase 2 of 4: implement the compatibility profile.

This PR adds the first-party ClickHouse-LDAP compatibility implementation as a new, dormant package internal/ldap/profile (~2.6k physical LOC production (2,659 measured), 34+ files with tests). Production is untouched: cmd/ch-oauth-ldap, the legacy non-test internal/ldap package files, third_party/** and go.mod/go.sum are byte-identical to main, and a committed dependency-contract test fails CI if the new package (or module-path cryptobyte) ever appears in the production closure before Phase 4's cutover. Rollback: revert the merge; runtime behavior is identical either way.

What the package implements (per the wire evidence from Phase 1)

• Bounded framing before allocation (frame.go): outer 0x30, definite minimal lengths only (≤3 length octets), 64 KiB body cap checked before the single allocBody call (test hook proves zero allocation on rejection, incl. the historical ~2 GiB declaration 30 84 7f ff ff ff); envelope requires a minimal positive MessageID (1..MaxInt32, shared validator reused by Abandon's [APPLICATION 16] IMPLICIT target), exactly one protocolOp, no trailing bytes; minimal Controls scanner returns only hasCritical. Malformed → close, no resync.
• LDAPv3 simple Bind (bind.go): clearAuth() before any validation on every recognizable Bind (failed re-Bind clears, success replaces — wholesale, roles cloned); empty DN/password → 49; version ≠ 3 → 2 (version 0/negative closes at decode, recorded as a narrowing); SASL [3] → 7; any other auth tag → close. The password exists as one bounded copy passed to the Verifier and is never stored or logged. Verify/Roles are call-site-unique to bind.go — enforced type-aware by internal/securitytest/profile_types_contract_test.go (go/types Selections over the fully type-checked package, stdlib export-data importer; rewritten from the original AST shape checks after review found syntax-enumeration bypasses), so unsupported operations structurally cannot reach OAuth verification.
• Fixed Search shape (search.go): structural AND(objectClass=groupOfNames, member=<current BoundDN>), either child order, duplicates/extras/nesting rejected; base/scope/deref/typesOnly per the captured profile (out-of-range ENUMERATED closes; in-range out-of-profile → 50); fail-closed size limit — at most N eligible entries then sizeLimitExceeded, exact boundary tested; member DN compared structurally (never suffix/text).
• Restricted DN grammar (dn.go): iterative RFC 4514 subset (single-attribute RDNs, simple + 2-hex escapes, UTF-8 validated, NUL rejected); Bind shape exactly <rdnAttr>=<user>,<UserBaseDN>; safe synthetic cn=<escaped role>,<GroupBaseDN> rendering. Parse errors carry no input bytes (offsets/lengths only).
• Result encoding (encode.go): BindResponse, SearchResultEntry (overflow-checked size preflight), SearchResultDone, minimal error result; MessageID round-trips exactly (127 → 02 01 7f, 128 → 02 02 00 80, asserted at byte level and replayed over TCP).
• Connection model (server.go): accept loop → one goroutine per admitted connection (cap 256; the 257th gets neither allocation nor goroutine), 30 s read/write deadlines, Abandon recognized/dropped with no response, Unbind/close, graceful shutdown with goroutine-settling tests.
• Logging (logging.go): closed 12-literal reason vocabulary (8 legacy literals byte-identical to the old package), constant-only diagnostic, correlation-ID hash byte-identical across backends — all AST-enforced.

Testing

• Both-backend parity: profile_compat_test.go drives the legacy internal/ldap.Server and the new profile.Server over real loopback TCP with the same fakes and inputs.
• Differential oracle vs vendored goldap (Phase-2-only, deleted at cutover): same profile-valid input through both decoders, normalized semantic comparison; committed wire fixtures + boundary encodings; narrowings excluded with reasons.
• Wire replay: every committed internal/ldap/testdata/clickhouse-wire/** session (24.8/25.8 × success/timeout-abandon + constructed 127/128) replayed over real TCP, with an independent filesystem walk so a skipped session can't hide.
• Native fuzzing: FuzzLDAPFrame, FuzzBindRequest, FuzzSearchRequest, FuzzRestrictedDN, FuzzMemberAssertionDN — seeds from the committed corpus, MessageID 127/128, the historical fork vulnerability shapes (2 GiB length declaration, INTEGER sign bug, filter-nesting allocation), truncated/oversized lengths, hostile DN escapes. 20 s smoke per target run clean by the coordinator.
• Adversarial/lifecycle: stalled header/body/write, mid-Search disconnects, 50-connection shutdown, connection-cap admission.
• Contracts: dependency (profile + cryptobyte absent from production closure; no general-LDAP modules in the profile's own closure), architecture — syntactic invariants by AST (profile_architecture_contract_test.go: single production goroutine statement, nonrecursive filter, constant-only reason/diagnostic, no legacy-LDAP imports) and type-semantic invariants by go/types (profile_types_contract_test.go: only-map Server.active, only-channels serveDone/stopDone, Verify/Roles exactly once in bind.go — incl. generic/alias/call-receiver forms), redaction inventory extended (internal/ldap/profile is an audited scopeDir; credential-reachable sinks carry marker proofs; nested-package guard).

Verification

• All five Required PR gate commands green locally plus go vet -tags phase5release ./internal/securitytest; go mod tidy is a no-op.
• Coordinator-run fuzz smoke (5 × 20 s) clean; production closure checked directly (go list -deps ./cmd/ch-oauth-ldap contains neither the new package nor module-path cryptobyte).
• Reviews: one High-risk read-only internal pass (no blocking findings; the write-stall classification fix, DN-error hardening, ASCII descriptor folding, and narrowing/LOC records landed as review fixes) and a security review (no findings ≥0.7; confirmed no stale-auth window, cap-before-allocation, credential lifetime bounded to the Verify call).
• LOC: 2,659 physical production LOC, measured and recorded (plan's 2,500 consolidation trigger tripped by the plan table's omitted config.go/doc.go; disposition recorded in the ship log — well below ADR ADR: Own the ClickHouse LDAP compatibility profile and remove general LDAP dependencies #32's ~3,500 architecture-review trigger; consolidation review folds into Phase 4's deletion).

Deferred (later phases, recorded in the ship log)

Phase 3: gates/matrix/HA on the replacement, LOC guardrail measurement as an official gate. Phase 4: cutover (cmd/ch-oauth-ldap switches to profile), deletion of third_party/** forks and production go-ldap, inversion of the dependency contracts, replacement of the test-only goldap oracles, deletion of the differential harness. The ten client-visible narrowings are recorded in docs/clickhouse-ldap-wire-profile.md §11.3 for Phase 3 acceptance.

🤖 Generated with Claude Code

https://claude.ai/code/session_011KxntXZk6BFoz3uT3wFtiS

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: d273414b4afb42aa551becbd776b0c84c8e5cd64

I reviewed the complete 47-file PR against #33 Phase 2, the current base (edebb6d2c08d0eb9d860c1909de8d5fc85d3c9bd), the Phase 1 wire/decoder evidence, the lifecycle/dependency/redaction contracts, and the supplied Phase-2 review contract. The exact-head Required PR gate Actions job is green. I also attempted to obtain a local checkout for focused execution, but this environment could not resolve github.com, so I did not treat local tests as independently rerun.

P1 — Pre-auth request parsing reintroduces proportional allocation amplification

internal/ldap/profile/search.go:156-166 converts every requested attribute to a fresh Go string and appends it to an unbounded-by-count []string before even checking c.authenticated. A single <=64 KiB Search can therefore encode tens of thousands of tiny/empty AttributeDescription OCTET STRINGs and turn that body into hundreds of KiB of slice metadata plus copied strings; 256 admitted connections can do this concurrently. internal/ldap/profile/dn.go:181-233,279-316 has a similar issue on credential-bearing Bind DNs: ParseDN copies the whole string to []byte, grows a decoded []byte and parallel []bool per value, then copies each decoded value into strings. handleBind also converts the DN from the frame to a string before ParseBindDN, so a near-cap hostile DN has several proportional copies before Verify.

That contradicts this unit's explicit resource invariant: 64 KiB must be the bounded request body and attacker-controlled bodies must not acquire proportional duplicate copies/allocation amplification. The frame allocator test cannot detect allocations made after readFrame succeeds.

Action: keep profile fields as slices/spans aliasing the already-bounded frame wherever possible. For attributes, there is no reason to build a slice: stream the SEQUENCE, retain/compare only the first attribute, count/reject a second, and compare cn without string(a). Rework the DN parser to scan the input string directly and allocate decoded storage only when escaping actually requires it; avoid a parallel bool-per-byte buffer and avoid reparsing/copying the same Bind DN. Add allocation-budget tests for a 64 KiB Search containing maximal tiny attributes and a near-cap hostile Bind DN, multiplied conceptually by the 256 admission cap.

P2 — Simultaneous sizeLimit/timeLimit precedence diverges from the legacy supported behavior

internal/ldap/profile/search.go:333-351 skips empty roles and then checks sizeLimit before checking whether the Search deadline expired. Legacy internal/ldap/search.go:113-134 checks time expiry first, then attempts rendering/skips an invalid role, then checks sizeLimit. Therefore after exactly N entries have been emitted, if the deadline expires before the next eligible role, legacy returns result 3 (timeLimitExceeded) while the profile returns result 4 (sizeLimitExceeded). This is a profile-valid input (both positive limits), is not one of the documented ten Phase-3 narrowings, and is not covered by the separate size-only/time-only tests.

There is a related empty-role edge: legacy checks the deadline before discovering/skipping the empty role; profile skips it first. That can also change which terminal result wins.

Action: restore the legacy observation order for profile-valid Search: test expiry at the top of each role iteration, render/skip, then apply the N+1 size-limit check. Add a deterministic clock test with both limits active where N entries are already emitted and expiry occurs immediately before role N+1, plus an empty-role variant. Put that case in the old/new compatibility suite, since it is not a declared narrowing.

P2 — The architecture contract is syntactic, not the mechanical no-registry/no-channel proof it claims to be

internal/securitytest/profile_architecture_contract_test.go:541-580 only discovers maps when they are explicit struct-field types or ValueSpec.Type is a *ast.MapType; : = make(map[...]) and inferred var x = make(map[...]) are invisible. The channel check at :626-667 has the same hole for ch := make(chan ...). A request registry/work queue could therefore be introduced with an inferred local map/channel and the advertised contract would remain green. The .Verify/.Roles uniqueness check is similarly selector-call based and can be bypassed with a method value (f := c.verifier.Verify; f(...)).

The current production code does not contain those forbidden structures; the finding is that the acceptance mechanism does not enforce the invariant it claims to enforce.

Action: either use go/types to resolve declared/local expression types and method ownership, or explicitly inspect AssignStmt/initializers for make(map...), make(chan...), composite map literals, and method-value selectors. Add the actual bypass forms above as sabotage tests, not only a Server field map/channel/direct .Verify(...) call.

P2 — ModifyDN log parity is broken by operation-name casing

internal/ldap/profile/logging.go defines opModifyDN = "modifydn"; current internal/ldap/unsupported.go logs "modifyDN". The PR says ordinary mapped unsupported-operation logging is preserved and this difference is not listed as a narrowing, so the new backend silently changes a log contract while returning the same result code.

Action: change the profile literal to modifyDN and add an exact unsupported-operation log comparison for ModifyDN so this cannot drift again.

P3 — §11.4 misstates which decoder is the independent Phase-1 oracle

docs/clickhouse-ldap-wire-profile.md:743-754 says TestClickHouseWireCryptobyteDecision “uses cryptobyte itself as the independent decoder proving fixture well-formedness,” then a few lines later correctly refers to replacing the independent goldap decoder. The Phase-1 test actually corroborates cryptobyte failures with independentlyWellFormedBER, implemented using the vendored goldap decoder (internal/ldap/clickhouse_wire_cryptobyte_test.go).

Action: rewrite the first sentence of §11.4 to say goldap is the independent well-formedness leg and the bounded Phase-4 cursor replaces that goldap leg. This matters because the supplied review contract explicitly requires changed prose to be true of the tree.

The runtime design is otherwise directionally aligned with ADR #32: the profile stays dormant, framing rejects oversize declarations before body allocation, dispatch remains synchronous, the filter decoder is fixed/nonrecursive, and the exact-head required CI gate is green. The findings above are specific gaps in resource enforcement, parity, mechanical proof, and documentation rather than a request to broaden the implementation.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previous reviewed head: d273414b4afb42aa551becbd776b0c84c8e5cd64

Reviewed head: ba8862d3928f529cf9785b0ff0e76becb39d5a67

The fix delta is one commit and changes only CLAUDE.md, docs/clickhouse-ldap-wire-profile.md, and internal/securitytest/profile_architecture_contract_test.go; the other 44 PR files are unchanged from pass 1. The exact-head Required PR gate is green. I could not obtain a local checkout because this environment cannot resolve github.com, so I did not claim an independent full repository test run; I did run a focused local Go-AST probe against the new guard logic described below.

P1 — Pre-auth Search parsing still amplifies a bounded body into proportional duplicate allocations

Status from pass 1: unchanged / still valid. internal/ldap/profile/search.go still decodes the entire AttributeSelection into []string before checking authentication or the fixed one-cn profile rule:

var attributes []string
for !attrsSeq.Empty() {
    var a []byte
    if !attrsSeq.ReadASN1Bytes(&a, asn1.OCTET_STRING) { ... }
    attributes = append(attributes, string(a))
}

Issue #33 explicitly says the 16 MiB figure is only the body-buffer bound and to avoid proportional duplicate copies of attacker-controlled message bodies. A near-cap Search containing many one-byte AttributeDescriptions can carry on the order of 20k attributes; the []string headers alone are hundreds of KiB for a single 64 KiB request, before the copied string bytes, and this happens before the eventual len(attributes) != 1 rejection. The frame allocator proof cannot see these post-frame allocations.

Action: do not materialize AttributeSelection. Stream the sequence, keep/compare only the first attribute, and reject as soon as a second appears; compare the bytes to cn without string(a). Add an allocation-budget regression for a near-cap many-attribute Search. The DN path may deserve a similar measured budget test, but the Search path alone is enough to reproduce the contract violation.

P2 — Simultaneous sizeLimit / timeLimit precedence still diverges on profile-valid Search

Status from pass 1: unchanged / still valid. The profile loop still skips empty roles, then checks sizeLimit, then checks expired(). Legacy checks expired() first, then render/skip, then sizeLimit.

Therefore, after exactly N entries have been emitted, if the client deadline expires before the next eligible role, legacy returns result 3 (timeLimitExceeded) while the profile returns result 4 (sizeLimitExceeded). An empty role at that point also changes observation order. Both positive limits are part of the supported ClickHouse-shaped request, and this divergence is not a documented narrowing.

Action: restore legacy observation order: deadline check at the top of each role iteration, render/skip empty/invalid role, then apply the N+1 size check. Add deterministic both-limits tests for expiry before role N+1 and for an empty-role edge, and put the non-empty case in the old/new compatibility suite.

P2 — The architecture fix closes the exact pass-1 examples but the claimed mechanical invariant is still bypassable

Status from pass 1: partially fixed, not closed. The new helpers correctly catch pending := make(map...), work := make(chan ...), and direct method-value extraction such as verify := c.verifier.Verify. The remaining detector is still syntax-pattern-specific:

• findMapTypedSites ignores var pending = make(map[int32]*request) because that is a ValueSpec with Type == nil, not an AssignStmt; it also ignores pending := map[int32]*request{} and named map types.
• findChanTypedSites similarly ignores var work = make(chan request).
• verifyOrRolesSelectorSites requires the receiver shape .verifier.Verify / .roles.Roles, so v := c.verifier; v.Verify(...) (and the analogous resolver alias) leaves the count at the one legitimate direct site.

I reproduced these against a minimal Go parser probe implementing the current helpers: the inferred var map/channel and map-literal cases produced zero detected sites, while adding a verifier/resolver receiver alias still produced only one counted Verify/Roles site.

Action: for maps/channels, the simplest robust AST rule is to reject every production *ast.MapType / *ast.ChanType node except the exact allowlisted declaration sites; that catches explicit declarations, make, composite literals, and named-type declarations. For Verify/Roles ownership, use go/types, or add a stronger field-flow rule rather than matching only the immediate receiver spelling. Add regression tests for var x = make(...), map composite literals, and receiver aliases.

P3 — ModifyDN log parity is still broken by casing

Status from pass 1: unchanged / still true, but lower severity than the resource/parity findings above. The profile still defines opModifyDN = "modifydn"; legacy logs "modifyDN". If exact operation-name log parity is intended, this remains a one-line drift.

Action: use modifyDN and add one exact log-parity assertion for ModifyDN.

P3 — The SASL/version rebuttal is correct, but pin it and qualify §11.3

The pass-2 context's rebuttal is supported by the legacy handler: legacy rejects AuthenticationChoice() != "simple" before any version policy, so a decodable v2/v4 SASL Bind gets result 7 there as it does in the profile. I therefore withdraw the pass-1-style claim that SASL must be reordered behind the version check.

The remaining gap is documentation/test precision: §11.3 currently says broadly that Bind version !=3 changes to result 2, but that is only true for the recognizable simple-Bind path; SASL stays result 7 regardless of version. The current profile tests cover v3 SASL and v1/v2/v4 simple separately, not the cross-product.

Action: add a v2-SASL regression test (optionally v4 too) and qualify §11.3 item 1 as the simple-Bind version narrowing. This prevents a future well-intentioned reorder from breaking the parity the rebuttal relies on.

Fixed from pass 1

• CLAUDE.md now correctly scopes the stdlib+cryptobyte claim to the ASN.1/protocol layer.
• §11.4 now correctly identifies vendored goldap as the independent well-formedness decoder that Phase 4's bounded cursor replaces.
• §11.3 now explicitly records the profile's version >=128 graceful-result widening versus legacy's 1..127 decode window.

No additional runtime regression was introduced by the one-commit fix delta; the blocking issues above are the still-open resource/parity contracts and the still-incomplete mechanical proof.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3

Previous reviewed head: ba8862d3928f529cf9785b0ff0e76becb39d5a67

Reviewed head: 1dbaa59a0b50a543cb5d065afdff18cc765c84c0

I compared the new head against pass 2, reassessed every prior finding, and reused the earlier full-PR review for the 42 files unchanged by this one-commit delta. The exact-head Required PR gate GitHub Actions run is green.

P2 — The architecture contract is still bypassable despite the new sabotage cases

The pass-3 change closes several concrete syntax holes, but the claimed mechanical no-map/no-channel and Verify/Roles uniqueness guarantees are still not actually exhaustive.

findMapTypedSites handles explicit types, var x = make(map...), x := make(map...), and map composite literals only in AssignStmt. Its ValueSpec branch with Type == nil accepts only CallExpr/make(map...), so the adjacent ordinary form var pending = map[int32]*request{} is still invisible. More generally, a helper such as func newPending() map[int32]*request { return make(map[int32]*request) } followed by pending := newPending() is also invisible: function result/return expressions are not inspected as map sites and the assignment RHS is not the builtin make. The channel detector has the same helper-return gap.

The Verify/Roles fix is similarly one alias deep. fieldAliasIdents records only identifiers assigned directly from <expr>.verifier/<expr>.roles; v := c.verifier; w := v; w.Verify(...) is not recorded for w. A typed parameter (func f(v Verifier) { v.Verify(...) }) also bypasses the receiver-shape heuristic entirely. These are ordinary Go refactorings, not exotic AST tricks, so the test still cannot support the PR/contract claim that unsupported paths are structurally unable to invoke OAuth.

Action: simplify the map/channel proof by scanning every production *ast.MapType/*ast.ChanType node and allowlisting only the exact sanctioned declarations; that automatically catches declarations, function params/results, returns, composite literals, named underlying types, and helper factories. For Verify/Roles, use go/types to resolve method ownership/call sites, or implement complete typed dataflow rather than a receiver-name heuristic. Add sabotage tests for var x = map[...]{};, helper-return map/channel creation, a second-level receiver alias, and a Verifier/RoleResolver-typed parameter.

P2 — The size/time runtime fix landed, but the promised two-backend regression case did not

executeSearch now matches legacy's ordering: expiry check first, then render/skip, then the N+1 size-limit check. The new deterministic profile-package test covers the simultaneous-limit + empty-role edge.

However internal/ldap/profile_compat_test.go is byte-unchanged from the pass-2 head. It still has separate TestProfileCompatibility_SizeLimit and TestProfileCompatibility_TimeLimit fast-success cases, but no simultaneous size/time boundary case. That leaves the exact divergence that triggered pass 2 unverified by the two-backend real-TCP parity harness, despite the pass-3 handoff saying it was added to the compat suite.

Action: add a shared old/new regression case for the simultaneous-limit precedence, or explicitly document why this edge cannot be made deterministic in the real-TCP harness and keep a separate legacy/profile deterministic proof that compares the same transition.

P3 — The new allocation regression test is not actually an admitted near-cap request

nearCapCount = maxBodyBytes/3 + 100 gives 21,945 one-byte attributes. Their OCTET STRING TLVs alone consume 65,835 bytes, already exceeding the 65,536-byte body cap before the SearchRequest's other fields and enclosing lengths are added. measureAttrSearchAllocBytes then invokes doSearch, which calls handleSearch directly and bypasses readFrame, so the test comment's claim that this is a near-cap request that fits the per-message cap is false.

This does not invalidate the streaming implementation itself, but it weakens the resource-bound regression proof.

Action: construct the largest full SearchRequest that remains <=64 KiB, preferably drive it through the real frame/read path, and assert both admission and flat allocation growth.

Earlier findings reassessed

• Pre-auth AttributeSelection materialization: fixed; the decoder now retains at most the first attribute and stops after the second settles the out-of-profile result.
• Simultaneous size/time runtime ordering: fixed in implementation; test coverage remains incomplete as above.
• v2/v4 × SASL: pass-1 finding remains withdrawn; legacy also rejects non-simple auth before any version policy. The new v1/v2/v4 SASL test and §11.3 wording now pin this correctly.
• modifydn vs legacy modifyDN: withdrawn. The profile explicitly owns a closed lowercase op vocabulary, while the byte-identical compatibility guarantee is stated for the legacy reason literals and correlation-ID derivation, not every legacy operation-name spelling.
• Pass-1 documentation corrections remain present.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: f3bf13bf1c44bd62dec26f9a9e08d2b3dad063a8

Base: edebb6d2c08d0eb9d860c1909de8d5fc85d3c9bd

I reviewed the complete current 49-file PR, reusing the earlier full parser/runtime review where the tree is provably unchanged. The re-certification delta from 685fdf005ab2a30de16eb4ae49fc374237f1c768 is one commit touching only five test/doc files; no profile runtime file changed. The exact-head Required PR gate run #32 is green. I also attempted a fresh clone, but this runtime cannot resolve github.com, so I did not claim an independent full local suite run. I did run a focused standalone go/types reproduction against the new checker's core logic.

Major — constrained type parameters still bypass the new map/channel type-semantic checker

Risk score: 82/100

internal/securitytest/profile_types_contract_test.go is a major improvement over the retired syntax-shape enumerator, but typeCarries still does not cover all ordinary Go type semantics it claims to cover. It handles pointer/slice/array/struct/tuple/signature/interface-method structure, but it has no *types.TypeParam handling and its *types.Interface branch examines only methods, not embedded type-set terms / unions.

That makes this valid ordinary Go invisible to collectTypedSites:

func hiddenMap[T ~map[int]int]() any { return make(T) }
func hiddenChan[T ~chan int]() any   { return make(T) }

func bad() (any, any) {
    return hiddenMap[map[int]int](), hiddenChan[chan int]()
}

Why it passes the current checker:

• inside each helper, make(T) has type T; T.Underlying() is its constraint interface, and the current interface walk sees zero methods and never inspects the embedded ~map[...] / ~chan ... type term;
• the helper signatures return any, so the function/result objects do not carry a map/channel according to the current walk;
• at the caller, each call expression has type any;
• the explicit map[...] / chan ... type arguments are type expressions, which collectTypedSites deliberately excludes with tv.IsType().

I reproduced the current typeCarries algorithm independently with go/types: both the map and channel examples report 0 detected sites. This is not reflection or unsafe, and it can allocate and retain new map/channel state behind any while both invariant tests remain green. It also falsifies the new file/CLAUDE wording that the checker covers every map/channel-typed declaration, signature, or value expression available to the Go type checker.

Action: make typeCarries traverse type-parameter constraints and interface type sets, including embedded types and union terms (e.g. *types.TypeParam.Constraint(), Interface.NumEmbeddeds/EmbeddedType, and *types.Union terms). Add the two examples above as synthetic regressions through the same collectTypedSites path used on the real package. A ~map/~chan constraint is the important discriminator; the existing generic identity test (T any instantiated at a concrete map) does not cover this case.

Minor — the canonical PR description is stale about the enforcement mechanism

The Bind section of the current PR description still says Verify/Roles uniqueness is “enforced by an AST architecture contract.” At this head those type-semantic invariants have intentionally moved to profile_types_contract_test.go / go/types; only the genuinely syntactic invariants remain in the AST checker. Please update the PR description so the review record matches the tree.

I found no new parser/runtime regression in this head: the runtime is byte-identical to the previously reviewed 685fdf... tree, and the earlier Bind/Search/allocation/parity fixes remain unaffected. The remaining blocker is the still-incomplete mechanical proof above.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: f3bf13bf1c44bd62dec26f9a9e08d2b3dad063a8

Reviewed head: aef938baab5a20d097d69fe6fe1c93c30847cc11

Base: edebb6d2c08d0eb9d860c1909de8d5fc85d3c9bd

The new head is exactly one commit ahead of the previous review and changes only internal/securitytest/profile_types_contract_test.go (+60). The parser/runtime and the other 48 PR files are byte-identical to the previously reviewed head. Exact-head Required PR gate run #34 is green.

P2 — The type-aware map/channel checker still misses instantiated named generic composites

The pass-1 constrained-type-parameter hole is fixed: typeCarries now walks interface embedded type-set terms and *types.Union, and the exact hiddenMap[T ~map...] / hiddenChan[T ~chan...] regressions are present.

A distinct ordinary-generics bypass remains. typeCarries checks whether a type's own underlying type is directly a map/channel, then immediately returns false for every other *types.Named. The file justifies that by saying an in-package named composite's components are caught at their declaration identifiers. That is not true for an instantiated generic composite whose declaration is unconstrained:

type Box[T any] struct { V T }

var hiddenMap  Box[map[int]int]
var hiddenChan Box[chan int]

At the declaration of Box, field V has type parameter T constrained only by any, so no map/channel is visible there. At each variable declaration the instantiated type is a *types.Named (Box[map[int]int] / Box[chan int]), so the current early named-type return makes the checker opaque before examining its type arguments or instantiated underlying struct. The concrete map/chan appears only inside the type expression, and collectTypedSites deliberately excludes tv.IsType() expressions.

I reproduced the current typeCarries logic independently with go/types using exactly the package-level declarations above: it reports maps=0 chans=0.

This contradicts the file's precise claim that there is no map- or channel-carrying declaration/value expression outside the allowlist “for every way such a type ... can appear to the Go type checker.” The current production package does not contain such a wrapper; this is an acceptance/drift-gate defect, not a runtime parser bug.

Action: before treating a non-map/non-channel *types.Named as opaque, inspect its instantiated TypeArgs() recursively for map/channel-carrying types (or equivalently traverse instantiated in-package named composites while preserving the intended external-type opacity). Add the exact Box[map[int]int] and Box[chan int] package-state forms as regressions through collectTypedSites.

P3 — bind.go still describes the Verify uniqueness gate as AST-based

The PR description has been corrected as requested in pass 1, but internal/ldap/profile/bind.go still says the sole Verifier.Verify call is “mechanically enforced elsewhere by an AST-based architecture contract.” That is now false: the relevant invariant moved to the go/types-based profile_types_contract_test.go.

Action: update that comment to name the type-aware contract. This is non-blocking by itself, but the repository explicitly treats prose/tree truth as part of the review contract.

Earlier pass-1 findings

• Constrained generic ~map / ~chan hole: fixed at this head.
• Stale PR description claiming AST enforcement: fixed in the canonical PR description.
• No new runtime/parser changes occurred between the two reviewed SHAs; prior runtime findings remain resolved/withdrawn as previously recorded.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed head: aef938baab5a20d097d69fe6fe1c93c30847cc11

Reviewed head: 78570aa59d1112e2247692484314c0edb6952f73

Base: edebb6d2c08d0eb9d860c1909de8d5fc85d3c9bd

I re-reviewed the current 49-file PR using the prior full-PR analysis, then inspected the exact one-commit delta from aef938b.... That delta changes only internal/securitytest/profile_types_contract_test.go plus a comment in internal/ldap/profile/bind.go; parser/server/Search/framing runtime behavior is unchanged. Exact-head Required PR gate run #35 is green.

Reassessment of pass-2 findings

1. Box[map]/Box[chan] generic-instantiation hole: fixed. typeCarries now walks *types.Named.TypeArgs() before the opaque-Named return, and the two exact regressions exercise collectTypedSites.
2. Stale bind.go AST-enforcement comment: fixed. It now points at the go/types contract.

P2 — Verify/Roles checker counts method references, not unique invocation sites

methodSelectionSites treats a method call, method value, and method expression as equivalent sites, and TestProfileArchitecture_VerifyAndRolesOnlyOnceInBind only requires exactly one matching selection whose position starts with bind.go:. That does not mechanically establish the PR's stronger claim that Verify/Roles are call-site-unique to Bind and that unsupported operations structurally cannot reach OAuth verification.

A type-safe counterexample that still passes the current invariant is:

type connection struct {
    verifier Verifier
    verify   func(string) error
}

// bind.go
func (c *connection) handleBind() {
    c.verify = c.verifier.Verify // the one and only Verify selection, in bind.go
}

// unsupported.go
func (c *connection) handleUnsupported() error {
    return c.verify("attacker") // invokes Verify, but creates no Verify selection
}

I reproduced this with the same go/types selection logic: it reports exactly one Verify site, bind.go, so the current test condition is satisfied while handleUnsupported can invoke the verifier arbitrarily. This is the same reason the existing regression test for method-value extraction is insufficient: it proves an additional extraction is counted alongside the current direct call, but it never tests replacing the direct call with the sole method-value selection and invoking that value elsewhere.

Action: make invariants 12/13 require the sole Verify/Roles selection to be a direct call expression in handleBind, and reject method-value / method-expression forms outright. Add a regression that replaces the legitimate call with a stored method value and invokes it from another handler; it must fail even though there is still exactly one selection in bind.go. If the intended claim remains the stronger "unsupported operations structurally cannot reach OAuth verification", also close dynamic escape routes (at minimum reflect, currently not forbidden) or narrow the prose claim to exactly what the checker proves.

P3 — file-level contract prose is stale after the TypeArgs fix

The header of profile_types_contract_test.go still says a named type is flagged only when its own underlying type is map/channel and "is otherwise opaque". The function-level comment/code now explicitly make generic named instantiations non-opaque by recursing through TypeArgs(). Update the file-level contract description so its stated proof matches the implementation.

No new parser/runtime regression was found; all earlier runtime fixes remain unaffected because this head changes no runtime logic.

VERDICT: REVISE