test(thread): close the integration-tag coverage gap on subchannel blacklist gates

[yujiawei]

背景

PR #345 review 的 P1 测试覆盖 finding（GH #353）。

checkChannelAccess 的 requireActive split（AuthorizeThreadFollow=active vs AuthorizeChannelFollow=permissive，#345 落地）只被 thread_follow_blacklist_e2e_test.go 覆盖，而该文件带 //go:build integration——CI 跑 go test 不带 -tags integration，文件永不编译。把 AuthorizeThreadFollow 翻回 permissive、重新打开 #345 刚关掉的缺口的回归，在 CI 会静默绿。

修法（issue 三个选项中的「补非 tagged 测试」）

• 新增 modules/message/check_channel_access_test.go（无 tag，CI 直接跑）：生产同款 newThreadAuthChecker 对真实 group/group_member/thread 表断言——requireActive=true 拒被拉黑成员、false 放行；被移出成员两档都拒；并钉住对外接线：AuthorizeThreadFollow 必须 active（被拉黑 → ErrThreadForbidden）、AuthorizeChannelFollow 保持 permissive
• 测试基建遵循本包非 tagged 用例的约定（PR fix(conversation_ext): exclude blacklisted members from thread-ext materialization (fanout + FollowChannel) #356 round-1 CI 红的教训）：不跑 sql-migrate——Migration=false 的裸 ctx + 手建最小表（DDL 与 e2e helper / 真迁移对齐）+ 裸 INSERT 种子，-shuffle=on 下与手建表用例（如 channel_files）任意排序都不会撞 Error 1050
• integration-tagged e2e 文件保留不动（本地深跑仍可用）；未选「加 -tags integration CI job」方案——非 tagged 单测更便宜且天然进既有 required check

顺带（同一 review 的 P2 项）

• 收紧 read_path_blacklist_test.go 四处裸 assert.NotEqual(200)：原断言会把 404/500 等无关失败当成「门禁生效」，现改为断言 wire=400（legacy D14 ResponseErrorL）+ not_group_member 文案
• 补 threadMdGet blacklist read 测试（GROUP.md 正文与列表/详情同级越权读面，此前无覆盖；deny 走 permission_denied）

与同批 follow-up 的关系

与 #356（GH#351 fanout 过滤）、#357（GH#352 bot_api 门禁）同属 PR #345 的强制 follow-up，三个 PR 文件互不相交、无合并顺序依赖。

审查状态

• 改动规模：+237/-4，2 文件（纯测试，无生产代码改动）
• 编译/vet：go vet（含 -tags integration）通过
• 测试：modules/message、modules/thread 全包 -race -shuffle=on 通过（CI 同款 MySQL/Redis/WuKongIM 栈；message 包另跑 fresh/dirty 两种 DB 状态 + 双 shuffle seed 验证排序无关）

Fixes #353

[yujiawei]

Code Review — PR #358 (octo-server)

Scope: test-only change, +237/-4 across 2 files. No production code is modified. Reviewed against head SHA 3b2fef69 with merge-base on main.

Summary

This PR closes a real coverage gap: the blacklist/active-membership gates introduced in #343/#345 were only exercised by //go:build integration e2e files, which CI's go test never compiles. A regression that reverted AuthorizeThreadFollow (or the read paths) back to permissive ExistMember would have passed CI silently. The two new/expanded un-tagged test files pin that behavior in the normal CI lane (which already has a MySQL service). This is a sound, well-targeted hardening of the test suite.

Verification against production code

All assertions match the current production implementation:

• modules/message/1module.go:144-191 — AuthorizeThreadFollow calls checkChannelAccess(..., requireActive=true) and translates ErrChannelForbidden → ErrThreadForbidden; AuthorizeChannelFollow calls with requireActive=false. The new TestAuthorizeFollow_BlacklistSplit and TestCheckChannelAccess_RequireActiveSplit assert exactly this split. ✅
• modules/message/1module.go:206-223 — requireActive=true → ExistMemberActive, false → ExistMember. ExistMemberActive (modules/group/db.go:182) adds status=GroupMemberStatusNormal, excluding GroupMemberStatusBlacklist (=2). The test's blacklist→deny / GROUP-branch→permissive expectations are correct. ✅
• modules/thread/api.go:620-628 — threadMdGet gates on ExistMemberActive and returns ErrThreadPermissionDenied for non-active members. The new TestReadPath_ThreadMdGet_BlacklistTransition asserts this exact code, distinct from the list/get/listMembers paths which use ErrThreadNotGroupMember (api.go:333/413/503). ✅
• Wire status — assertBlacklistDenied asserts http.StatusBadRequest even though both codes are HTTPStatus: 403. This is correct: respondThreadError → httperr.ResponseErrorL pins the wire status to 400 for D14 legacy compatibility (pkg/httperr/respond.go:22), with the real status carried in error.http_status. ✅
• Hand-built thread table includes last_message_at, which QueryActiveByGroupShortIDs explicitly SELECTs (modules/thread/db.go) — so AuthorizeThreadFollow's thread-existence query won't fail on a missing column. ✅

Quality notes

• The tightened assertBlacklistDenied is a genuine improvement over the prior bare assert.NotEqual(200), which would have counted unrelated 404/500 failures as "gate working." Asserting both the 400 wire status and the expected DefaultMessage substring correctly narrows the deny to the membership gate.
• The DROP TABLE + CREATE TABLE (rather than IF NOT EXISTS) and explicit Migration=false approach is the right call for the un-tagged lane: it sidesteps the -shuffle=on / sql-migrate Error 1050 collision documented from #356, and each test seeds its own minimal tables. CI runs packages serially with a fresh DB per package and these tests don't call t.Parallel(), so the destructive DDL is safe under shuffle.

Build/compile

go vet ./modules/message/ ./modules/thread/ passes cleanly — the new files type-check and reference only existing symbols (convext.ErrThreadForbidden, convext.ErrChannelForbidden, errcode.ErrThreadNotGroupMember, errcode.ErrThreadPermissionDenied, common.GroupMemberStatusBlacklist/Normal, newThreadAuthChecker).

Findings

No P0/P1/P2 issues.

Optional nits (non-blocking)

• nit: TestCheckChannelAccess_RequireActiveSplit and TestAuthorizeFollow_BlacklistSplit build their own MySQL *config.Context via config.New() default DSN rather than going through testutil.NewTestServer(). This is the deliberate (and documented) choice to avoid the migration collision, but it does couple the test to the default DSN matching the CI service. Already CI-proven, so just noting the implicit dependency.
• nit: The removed member denied on both subtest sets is_deleted=1 after the member was already blacklisted in the same test function's shared ctx. The assertions remain correct (is_deleted dominates both branches), but the layered state makes the subtest slightly less isolated than a fresh seed would be. Cosmetic only.

Security-sensitive note

This PR was flagged security_sensitive (membership/blacklist gate keywords). For a human verifier: the value of this change is precisely that it makes the blacklist read/follow gates fail closed in the standard CI lane. Worth confirming the new tests are actually included in the CI go test matrix for modules/message and modules/thread (they carry no build tag, so they should be) — once they run green there, any future permissive-membership regression on these paths will break the build instead of slipping through.

Verdict

Test-only, accurate against production, compiles, no blocking issues. Approve.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

PR#358 Review Report — test(thread,message): close the integration-tag coverage gap on blacklist gates

Reviewer: Octo-Q (automated review)
Head SHA: 3b2fef6906c0314b2169de69d821b46450c7ed17
Scope: +237/-4, 2 files (pure test, zero production code changes)
Routing: complexity=security_sensitive (automated review)

---

1. Verification Summary

Item	Status	Evidence
checkChannelAccess requireActive=true calls ExistMemberActive	✅	1module.go:206-270 — requireActive branch selects ExistMemberActive vs ExistMember
ExistMemberActive excludes blacklisted (status=2)	✅	group/db.go:182-189 — SQL WHERE status=GroupMemberStatusNormal(1)
ExistMember permissive (only is_deleted=0)	✅	group/db.go:171-175 — SQL WHERE is_deleted=0, no status filter
AuthorizeThreadFollow passes requireActive=true	✅	1module.go:144-175 — calls checkChannelAccess(…, true), translates ErrChannelForbidden → ErrThreadForbidden
AuthorizeChannelFollow passes requireActive=false	✅	1module.go:189-191 — calls checkChannelAccess(…, false)
Read-path handlers return wire=400 via ResponseErrorL	✅	thread/api.go:97 → httperr/respond.go:22-24 → respondL with useSemanticStatus=false → TransportStatus=400
threadMdGet uses ErrThreadPermissionDenied (not ErrThreadNotGroupMember)	✅	thread/api.go:627 — different error code from the other four read paths
Route /v1/groups/:group_no/threads/:short_id/md registered for threadMdGet	✅	thread/api.go:171

2. Findings

No P0/P1 findings.

P2 — ccaSpaceID only exercises legacy wildcard space-visibility path

Diff-scope: new (introduced by this PR's test setup).

setupCheckChannelAccessData seeds the group with space_id='' (legacy wildcard visible everywhere), while the test passes ccaSpaceID="s_cca". This means checkChannelAccess always short-circuits at the parentSpaceID == "" → return nil branch (1module.go:~250). The space-matching and external-group paths are never exercised.

Impact: Not a correctness issue for this PR's stated goal (pinning the requireActive split). The space-visibility paths have separate coverage from the integration-tagged e2e tests. Noting as a coverage observation for future consideration.

P2 — Subtests within TestCheckChannelAccess_RequireActiveSplit share mutable DB state

Diff-scope: new (introduced by this PR).

The three subtests (normal member passes both → blacklisted member denied only when requireActive → removed member denied on both) run sequentially and depend on cumulative state mutations (status→blacklist, then is_deleted→1). This is intentional and correct for the progression being tested, but means a failure in an earlier subtest leaves later subtests in an unexpected state. Standard Go testing pattern; noting only for documentation clarity.

3. Data-Flow Tracing

File 1: check_channel_access_test.go

TestCheckChannelAccess_RequireActiveSplit:

1. setupCheckChannelAccessData → seeds group (status=1, space_id=''), group_member (status=Normal, is_deleted=0), thread (status=1)
2. newThreadAuthChecker(ctx) → production constructor → group.NewService(ctx) + thread.NewDB(ctx) + group.NewDB(ctx)
3. Normal member: checkChannelAccess(…, true) → ExistMemberActive(groupNo, uid) → SQL count(*) WHERE is_deleted=0 AND status=1 → count=1 → true → passes space check (legacy wildcard) → nil ✅
4. After blacklist (status=2): ExistMemberActive → count=0 → false → ErrChannelForbidden ✅; ExistMember → count=1 (is_deleted still 0) → true → passes → nil ✅
5. After is_deleted=1: Both ExistMemberActive and ExistMember → count=0 → ErrChannelForbidden ✅

TestAuthorizeFollow_BlacklistSplit:

1. Same setup. Normal member: AuthorizeThreadFollow → checkChannelAccess(true) → nil → QueryActiveByGroupShortIDs → thread found (status=1 ≠ 3) → nil ✅
2. After blacklist: checkChannelAccess(true) → ErrChannelForbidden → translated to ErrThreadForbidden ✅
3. AuthorizeChannelFollow → checkChannelAccess(false) → ExistMember passes → nil ✅

File 2: read_path_blacklist_test.go

assertBlacklistDenied data flow:

1. Handler calls ExistMemberActive(groupNo, loginUID) → false for blacklisted user
2. Handler calls respondThreadError(c, errcode.ErrThread*, nil)
3. → httperr.ResponseErrorL(c, code, nil, details) with useSemanticStatus=false
4. → respondL sets TransportStatus=400 (legacy D14)
5. → Default renderer: JSON(400, {"msg": "<DefaultMessage>", "status": 400})
6. Test asserts w.Code==400 + body contains DefaultMessage ✅

TestReadPath_ThreadMdGet_BlacklistTransition:

1. Normal member GET /md → ExistMemberActive → true → handler returns 200 ✅
2. After blacklist → ExistMemberActive → false → respondThreadError(c, ErrThreadPermissionDenied, nil) → wire=400 + "You do not have permission to perform this action." ✅

4. Severity Rubric Application

• R1 (P1 hard definition): No finding makes a working path unavailable, produces wrong user-visible data, or makes things worse. N/A.
• R2 (diff-scope): Both P2 findings are new (introduced by this PR's test code), but neither represents a defect — they are coverage observations.
• R3 (distributed side-effects): N/A — pure test PR, no notification/push/event/count changes.
• R4 (verdict mapping): No P0/P1 → APPROVE is correct per rubric.

5. R5 Blind-Spot Checklist (security_sensitive routing)

• C1 — Dual-path parity: N/A. This is a test-only PR. The production requireActive split it tests was reviewed in PR#345. The tests correctly verify both paths (active vs permissive) behave as designed.
• C2 — Control-flow ordering / nesting reuse: N/A. No production control flow changed.
• C3 — Authorization boundary ≠ capability boundary: N/A for production code. The tests correctly verify that the authorization boundary (blacklisted members denied on active paths, allowed on permissive paths) matches the design.

6. Conclusion

Well-constructed test PR that precisely closes the CI coverage gap identified in #353. All test assertions verified against production code — error codes, wire formats, and data flows are correct. The tightened assertBlacklistDenied helper is a meaningful improvement over the previous bare NotEqual(200) assertions. The new threadMdGet blacklist test covers a previously untested read surface.

[Octo-Q] verdict: APPROVE — no P0/P1 findings; two P2 coverage observations noted for future consideration.

[yujiawei]

Code Review — PR #358 (octo-server)

Verdict: APPROVED

Test-only change (+261/-4, 2 files, no production code touched) that closes a real CI coverage gap on the subchannel blacklist gates. The work is correct, the new test infrastructure is sound, and — most importantly — the regression guard actually catches the regression it claims to. No P0/P1 blockers.

Scope verified

Reviewed at head 615aba5e3bc5a1798657fdd86b0f200a8cc8236e against merge-base main.

Item	Result
go vet ./modules/message ./modules/thread (with and without -tags integration)	✅ pass
go test ./modules/message ./modules/thread (CI-equivalent, no integration tag)	✅ pass
go test ./modules/message -shuffle=on (full package, ordering-independence)	✅ pass
Mutation test (see below)	✅ regression detected

Why this PR matters (the gap it closes)

The requireActive split landed in #345 (AuthorizeThreadFollow → active membership via ExistMemberActive; AuthorizeChannelFollow → permissive ExistMember) was only covered by thread_follow_blacklist_e2e_test.go, which carries //go:build integration. CI runs go test without -tags integration, so that file never compiles and the coverage was silently absent. A future change flipping AuthorizeThreadFollow back to permissive would have gone green in CI.

This PR adds modules/message/check_channel_access_test.go (no build tag → runs in the default CI job) that exercises the production newThreadAuthChecker against real group/group_member/thread tables.

Correctness checks

1. Production logic matches the assertions. Confirmed in modules/message/1module.go:

   • AuthorizeThreadFollow calls checkChannelAccess(..., true) and translates ErrChannelForbidden → ErrThreadForbidden (1module.go:154-160).
   • AuthorizeChannelFollow calls checkChannelAccess(..., false) (1module.go:190).
   • checkChannelAccess branches ExistMemberActive vs ExistMember on requireActive (1module.go:213-217).
   • ExistMemberActive adds status = GroupMemberStatusNormal on top of is_deleted = 0 (group/db.go:182-188), so blacklisted (status=2) and removed (is_deleted=1) members are both excluded — matching the removed member denied on both and blacklisted member denied only when requireActive subtests.

2. The guard is non-vacuous (mutation-tested). I locally flipped 1module.go:154 from checkChannelAccess(uid, spaceID, groupNo, true) to false (the exact regression #353 warns about) and TestAuthorizeFollow_BlacklistSplit failed with 被拉黑父群成员 follow 子区必须被拒 — An error is expected but got nil. Reverted afterward. This is the key signal that the new test is worth its weight.

3. Read-path tightening is correct. In modules/thread/read_path_blacklist_test.go, the four bare assert.NotEqual(200) were replaced with assertBlacklistDenied asserting wire status 400 + the deny message. Verified against production:

   • respondThreadError → httperr.ResponseErrorL is pinned to wire 400 (D14 compat), so asserting http.StatusBadRequest is correct even though the codes' real HTTPStatus is 403 (thread/api.go:88-89).
   • list/detail/members deny with ErrThreadNotGroupMember (api.go:333/413/503/924); threadMdGet denies with ErrThreadPermissionDenied (api.go:627) — the test uses the matching DefaultMessage for each path.
   • The new TestReadPath_ThreadMdGet_BlacklistTransition covers a previously-untested over-read surface (GROUP.md body), and threadMdGet does gate on ExistMemberActive (api.go:620), so the coverage is real.

Test-infra design (reviewed, sound)

The new file deliberately avoids module.Setup/sql-migrate and instead creates an isolated database (octo_msg_blacklist_test), hand-builds the three minimal tables, and DROP+CREATEs per case. The minimal DDL matches the columns the production queries actually read:

• thread includes last_message_at because QueryActiveByGroupShortIDs selects it (thread/db.go).
• GetGroups → QueryWithGroupNos does SELECT * from group, satisfied by the minimal group schema (status/space_id present, so the Disband and Space-visibility branches are reachable).

This isolation is the right call: the shared test DB is used concurrently by sibling packages via testutil.NewTestServer, and destructive DDL on it under go test ./... would collide. Confirmed the file's claim that message-package non-tagged tests don't call NewTestServer (the only reference in api_i18n_test.go is a comment, not a call).

Security note (security_sensitive classification)

This PR strengthens the security posture rather than changing runtime behavior: it has no production code changes, only adds regression coverage that prevents the blacklist gate from silently regressing in CI. Nothing here needs manual production verification beyond confirming the tests run in the standard CI job (they do — both new files are untagged).

Non-blocking observations (P2 / nit)

• Isolated DB is never dropped. ccaNewCtx CREATE DATABASE IF NOT EXISTS octo_msg_blacklist_test but never drops it; it persists across runs. Idempotent (each case DROP+CREATEs its tables), so this is harmless, but it does leak a database on the test host. Optional cleanup via t.Cleanup would be tidier.
• Cross-PR DB-name coupling. The comment says the isolated DB name is "shared with thread_ext_blacklist_filter_test.go (PR #356)". That sibling file does not exist at this head, so the coupling is latent. If #356 lands with conflicting DDL on the same DB name, ordering could matter — but since both DROP+CREATE per case and a package's tests run serially, it should stay safe. Worth a glance when #356 merges.
• CI host hygiene (not this PR). On my run the shared test DB was in a dirty migration state (tables present, gorp_migrations row missing → Error 1050 from testutil.NewTestServer). This is an environment issue, unrelated to this change; after resetting test the thread tests pass cleanly. Flagging only so it isn't misattributed to this PR.

[Jerry-Xin]

Summary: This PR is in scope for octo-server and adds relevant non-integration coverage for the thread/channel blacklist authorization split, with no blocking correctness issues found.

💬 Non-blocking

• 🟡 Warning: modules/message/check_channel_access_test.go:63-65 hardcodes root:demo@tcp(127.0.0.1:3306) for the isolated DB connection unless MSG_BLACKLIST_TEST_MYSQL_ADDR is set. Since the boot connection uses config.New(), this can fail in environments where the normal test DB is configured through the project’s standard config/env but not through the new custom env var. Consider deriving the isolated-DB DSN from the configured MySQL address and only replacing the database name.
• 🔵 Suggestion: The new test comments and assertion messages are very detailed but mostly non-English, for example modules/message/check_channel_access_test.go:3 and modules/thread/read_path_blacklist_test.go:71. For a public OSS repository, English comments and failure messages would make future failures easier for external contributors to diagnose.

✅ Highlights

• The new checkChannelAccess tests exercise the production newThreadAuthChecker against real DB-backed group, group_member, and thread behavior, which directly closes the non-tagged CI coverage gap.
• The tightened blacklist read-path assertions in modules/thread/read_path_blacklist_test.go:76 avoid the previous false positive where any non-200 response could pass.
• The added threadMdGet blacklist test covers a previously exposed read surface and matches the current ErrThreadPermissionDenied behavior.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

octo-server PR#358 Review Report

Reviewer: Octo-Q (automated review)
PR: #358
Head SHA: 615aba5e3bc5a1798657fdd86b0f200a8cc8236e
Title: test(thread): close the integration-tag coverage gap on subchannel blacklist gates
Scope: Pure test PR — +265/-4, 2 files, zero production code changes

---

1. Verification Summary

Item	Status	Evidence
requireActive=true → ExistMemberActive (excludes blacklist)	✅	1module.go:218-219 → group/db.go:182-188 (SQL: is_deleted=0 AND status=1)
requireActive=false → ExistMember (permissive)	✅	1module.go:220-221 → group/db.go:171-175 (SQL: is_deleted=0 only)
AuthorizeThreadFollow uses requireActive=true	✅	1module.go:160
AuthorizeChannelFollow uses requireActive=false	✅	1module.go:191
ErrChannelForbidden → ErrThreadForbidden translation	✅	1module.go:163-165
Wire format: ResponseErrorL → HTTP 400 (legacy D14)	✅	api.go:88-90 → respond.go:22-24,55-76 (transportStatus=400)
threadMdGet uses ExistMemberActive + ErrThreadPermissionDenied	✅	api.go:621-629
assertBlacklistDenied asserts wire=400 + error message	✅	Matches ResponseErrorL chain + defaultErrorRenderer output
Isolated DB (octo_msg_blacklist_test) prevents cross-package DDL collision	✅	ccaNewCtx creates dedicated DB; ccaEnsureTables DROPs/CREATEs within it
Legacy wildcard (space_id='') → visible everywhere	✅	1module.go:252-254 (parentSpaceID == "" → return nil)

2. Findings

P0/P1: None

All test assertions correctly match production code behavior. Data-flow tracing confirms:

• Membership check data flow: Test seeds group_member with status=GroupMemberStatusNormal(1) → ccaSetMemberStatus flips to GroupMemberStatusBlacklist(2) → ExistMemberActive SQL WHERE status=1 returns count=0 → isMember=false → ErrChannelForbidden → translated to ErrThreadForbidden in AuthorizeThreadFollow. Every step verified against production SQL at group/db.go:182-188. ✅

• Permissive path data flow: Blacklisted member (status=2, is_deleted=0) → ExistMember SQL WHERE is_deleted=0 returns count>0 → isMember=true → space visibility: group space_id='' → legacy wildcard → return nil. Test correctly asserts no error. ✅

• Removed member data flow: is_deleted=1 → both ExistMemberActive and ExistMember filter on is_deleted=0 → count=0 → isMember=false → ErrChannelForbidden. Test correctly asserts error on both requireActive modes. ✅

• threadMdGet data flow: ExistMemberActive returns false for blacklisted → respondThreadError(c, ErrThreadPermissionDenied, nil) → ResponseErrorL → wire HTTP 400 + body {"msg":"You do not have permission to perform this action.","status":400}. Test asserts http.StatusBadRequest + Contains(body, DefaultMessage). ✅

• Read-path deny tightening (4 existing tests): Previously bare assert.NotEqual(200) could false-positive on 404/500. Now asserts wire=400 + specific ErrThreadNotGroupMember.DefaultMessage ("You are not a group member."). All four read-path handlers (listThreads, getThread, listMembers, getThreadSimple) use respondThreadError(c, ErrThreadNotGroupMember, nil) at api.go:333/413/503/924. ✅

P2: Stale cross-PR reference in comment

File: check_channel_access_test.go:47-49
Issue: Comment states "与 thread_ext_blacklist_filter_test.go（PR #356）同名复用一个隔离库" — but thread_ext_blacklist_filter_test.go does not exist in the current codebase (on main). The ccaDBName = "octo_msg_blacklist_test" string also appears nowhere else.

Diff-scope: new (introduced by this PR). Not blocking — the comment is informational, and if PR #356 lands with that file using the same DB name, the two files are in the same package (sequential execution) so no collision occurs. But if PR #356's file is renamed or the DB name changes, this comment becomes misleading.

Suggestion: Consider either (a) softening the reference to "PR #356 如落地同名文件" or (b) confirming PR #356's final file name before merge.

Nits (non-blocking)

• shortID = "1489104291682713604" is a hardcoded production-format ID. Works correctly but could use a more obviously synthetic value for test readability.
• ccaNewCtx creates a boot context (for CREATE DATABASE) that is not explicitly closed. Acceptable in test code (GC handles it), but a defer on the underlying DB would be cleaner.

3. Suggestions

• Address the P2 stale-reference comment (low effort, prevents future confusion).
• No other changes needed — the test design is sound.

4. Additional Observations

• Schema drift risk (acknowledged tradeoff): Hand-built minimal tables in ccaEnsureTables must track real migration schema. The PR explicitly acknowledges this and aligns columns with e2e helpers. Acceptable for the isolation benefit.
• Test infrastructure quality: The isolated-DB approach (CREATE DATABASE IF NOT EXISTS octo_msg_blacklist_test + DSN swap) is well-designed — prevents the Error 1050 collisions that plagued earlier attempts (PR #356 round-1 CI red). Good pattern for future non-tagged tests that need destructive DDL.

5. Data-Flow Trace (consumed data → upstream source → runtime verification)

Consumed Data	Upstream Source	Reaches Consumer?
ExistMemberActive result	group/db.go:182-188 SQL: count(*) WHERE is_deleted=0 AND status=1	✅ Verified: blacklisted (status=2) → count=0 → false
ExistMember result	group/db.go:171-175 SQL: count(*) WHERE is_deleted=0	✅ Verified: blacklisted (status=2, is_deleted=0) → count>0 → true
GetGroups → SpaceID	group table space_id column	✅ Test seeds space_id='' → legacy wildcard → nil return
GetGroups → Status	group table status column	✅ Test seeds status=1 → not Disband → passes
QueryActiveByGroupShortIDs → thread map	thread table, key=groupNo____shortID	✅ Test seeds matching group_no + short_id + status=1
ErrChannelForbidden → ErrThreadForbidden translation	1module.go:163-165 errors.Is check	✅ Direct sentinel comparison, no wrapping
ResponseErrorL wire status	respond.go:55-76 transportStatus=400 when useSemanticStatus=false	✅ respondThreadError calls ResponseErrorL (not WithStatus)
ErrThreadPermissionDenied.DefaultMessage	errcode/server.go:52-56 = "You do not have permission to perform this action."	✅ threadMdGet handler at api.go:629
ErrThreadNotGroupMember.DefaultMessage	errcode/server.go:47-51 = "You are not a group member."	✅ Read-path handlers at api.go:333/413/503/924

6. R5 Blindspot Checklist (security_sensitive routing)

• C1 — Dual-path parity: N/A. Pure test PR. The tests themselves verify the production dual-path (requireActive=true vs false) is correctly wired at both AuthorizeThreadFollow and AuthorizeChannelFollow. The test assertions are symmetric with the production contract. ✅
• C2 — Control-flow ordering / nested reuse: N/A. Test helpers (ccaNewCtx, ccaEnsureTables, setupCheckChannelAccessData) are only called within this test file. No nested/reused security controls affected. ✅
• C3 — Authorization boundary ≠ capability boundary: N/A. No production authorization boundaries modified. Tests correctly verify existing boundaries. ✅

---

[Octo-Q] verdict: APPROVE — No P0/P1 findings. Tests correctly verify the requireActive split from PR #345, with proper data-flow alignment to production SQL, error translation, and legacy D14 wire format. One P2 (stale cross-PR comment reference) is non-blocking.

[lml2468]

Octo-PR APPROVE — #358 (head 615aba5)

经维护者授权补第三票。纯测试 PR,核实正确:

• ✅ 把 blacklist gate 从「仅 integration tag(CI 不跑)」补成 CI 可执行单元测试:checkChannelAccess requireActive 双语义、AuthorizeThread/ChannelFollow 入口接线钉住,断言精确(errors.Is)。
• ✅ 用专用隔离库 octo_msg_blacklist_test(与 #356 同名复用,两文件同属 message 包、go test 同包串行,当前安全)。
• 🟡 nit:共用隔离库名的"同包串行才安全"前提隐含、DSN 硬编码(有 env override)、注释英文化——均非阻塞 follow-up。

补上贯穿 #345/#356/#357 的"逻辑对但 CI 没跑过"覆盖缺口。无 blocking。
— Octo-PR(授权补票)