docs(searchmsg): fix producer attribution in package comment

[yujiawei]

What

Fix a stale attribution in the contract/searchmsg package comment.

The comment described the Kafka producer as octo-server's searchetl,
but the producer now lives in the standalone octo-search-indexer
image's internal/producer. The old wording could mislead readers into
thinking the producer is still built into octo-server.

Changes

• Corrected the producer attribution in the package doc comment
  (octo-server → octo-search-indexer), and updated the matching
  counterfactual in the placement rationale.

Out of scope

• No changes to any searchmsg struct, field, or SchemaVersion
  (the contract itself is untouched).
• Comment-only edit; no code logic, DB, or deployment changes.

Verification

• git diff contains only the package comment in
  contract/searchmsg/searchmsg.go (3 lines).
• go build ./... && go vet ./... → exit 0.

[lml2468]

🟡 REQUEST CHANGES (docs-only, non-urgent) — this comment fix makes the package less internally consistent, not more.

Build/vet/fmt all clean — zero code risk, it's a comment-only change. But the edit doesn't achieve its stated goal ("fix stale producer attribution") and introduces two new problems:

1. It creates a four-way contradiction within the same package. This PR changes searchmsg.go to say the producer lives in octo-search-indexer, but leaves the three sibling files saying the opposite:

• searchmsg.go:3 (this PR): producer = octo-search-indexer
• visibility.go:20 (unchanged): "octo-server searchetl producer (实时 + backfill 富化)"
• visibility_vectors.go:5 (unchanged): "octo-server modules/searchetl 的 producer"
• visibility_test.go:10 (unchanged): "octo-server searchetl 与 octo-search-indexer backfill"

Before this PR the whole package consistently said octo-server. After it, one file says indexer and three say server. For a PR whose entire purpose is attribution accuracy, that's a net regression — if the producer attribution is being corrected, all four sites must move together.

2. The new wording is internally self-contradictory in the two edited lines. It now reads: "契约被两侧 import：独立镜像 octo-search-indexer 的 producer（往 Kafka 写），以及 es-indexer consumer（从 Kafka 读）" — but es-indexer is the octo-search-indexer image (the rest of the package uses those names for the same standalone consumer image). Putting both the producer and the consumer inside octo-search-indexer destroys the doc's own core argument:

• The whole rationale for the shared contract is "producer and consumer are in different images, so the struct must live in octo-lib where both can import it." If producer and consumer are the same image, the Kafka decoupling and the "two sides import" framing no longer make sense.
• The placement counterfactual is now broken: "若放 octo-search-indexer，独立镜像的 es-indexer 无法 import" — that sentence is incoherent once the producer is also said to be in octo-search-indexer (it would be importing itself).

Root cause (and why I'm flagging not nitpicking): this is the third different producer attribution in this series — #76 said octo-server searchetl, #81's commit msg said "producer retired in prior phases," now #82 says octo-search-indexer. The doc keeps changing because the actual ownership decision isn't settled. A comment edit can't paper over that.

Ask: before merging, @yujiawei please pin down the real topology in one sentence and apply it to all four sites consistently:

• Where does the producer actually run? (octo-server searchetl? octo-search-indexer internal/producer? both have been claimed)
• Where does the consumer run? (the es-indexer / octo-search-indexer image)
• If producer and consumer really are now both in octo-search-indexer, then the "must live in octo-lib because two different images import it" rationale needs rewriting too — the justification would shift to "octo-server's searchetl also references it" or similar.

This is non-blocking for any runtime concern (comments only) and not urgent, but I'd hold the merge of this specific doc PR until the attribution is made consistent package-wide — otherwise we're shipping a "fix" that leaves the package contradicting itself in four places. Happy to flip to approve as soon as all four sites agree.

[Jerry-Xin]

Summary: The PR is relevant to octo-lib and safely updates documentation for the shared contract/searchmsg Kafka contract without changing behavior.

💬 Non-blocking

• 🔵 Suggestion: Nearby docs still reference the old producer location in contract/searchmsg/visibility.go:20, contract/searchmsg/visibility_vectors.go:5, and contract/searchmsg/visibility_test.go:10. If the producer has fully moved to octo-search-indexer, consider updating those in a follow-up for consistency.

✅ Highlights

• The changed file is part of this repository’s shared contract surface, so the PR passes the project relevance gate.
• No runtime, schema, serialization, or compatibility behavior changed.
• Verified git diff --check and go test ./...; both passed.

[mochashanyao]

[Octo-Q · automated review]

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

---

octo-lib PR#82 Review Report

Reviewer: Octo-Q (automated review)
PR: #82 — docs(searchmsg): fix producer attribution in package comment
Head SHA: 39f4f75431385fff494b9308e041b7c308570c37
Scope: Comment-only change, 1 file (contract/searchmsg/searchmsg.go), +3/−3 lines

---

1. Verification

Item	Status	Evidence
Producer now in octo-search-indexer	✅ Verified	octo-search-indexer/internal/producer/ contains kafka.go, etl.go, dlq.go, etc.
Producer removed from octo-server	✅ Verified	gh search code "searchmsg" --repo octo-server → 0 results; no search* dir in octo-server/internal/
octo-search-indexer imports octo-lib	✅ Verified	go.mod → github.com/Mininglamp-OSS/octo-lib v0.0.0-20260618092744-45384c784102
Both producer & consumer reference searchmsg	✅ Verified	internal/producer/{dlq,chunk,config}.go, internal/consumer/consumer.go, internal/esindex/{doc,writer}.go
No code logic / struct / field changes	✅ Verified	Diff is purely comment text; Message struct, SchemaVersion, constants untouched

2. Findings

F1 — Stale "两侧 import" framing (P2 / note)

Diff-scope: pre-existing, marginally amplified by this PR.

The comment still reads "该契约被两侧 import" (imported by two sides), implying two separate repos. In reality, both producer (internal/producer/) and consumer (internal/consumer/) now live in the same repo (octo-search-indexer). gh search code "searchmsg" --repo octo-server returns zero results — octo-server does not import this contract at all.

The PR's scope is explicitly limited to fixing the producer attribution (octo-server → octo-search-indexer), which it does correctly. The "两侧" framing is a pre-existing inaccuracy that this PR makes slightly more visible but does not create.

F2 — Stale placement rationale (P2 / note)

Diff-scope: pre-existing, marginally amplified.

The counterfactual "若放 octo-search-indexer，独立镜像的 es-indexer 无法 import" is now weak: since both producer and consumer are in octo-search-indexer, they could share types within the same repo. The real reason for keeping the contract in octo-lib (versioning discipline, neutral shared location for potential future consumers) is not stated.

The PR updates octo-server → octo-search-indexer in this rationale (line 5), which is the correct factual fix, but the underlying logic of the counterfactual no longer holds.

F3 — "三仓共享 config 宿主" stale (P2 / note)

Diff-scope: pre-existing, not touched by this PR.

Line 5 still says "三仓共享 config 宿主" but only two repos (octo-search-indexer, octo-server) import octo-lib, and only one (octo-search-indexer) uses searchmsg. Not in PR diff scope.

3. Suggestions

• F1/F2 follow-up (not blocking): Consider a follow-up that rewrites lines 3–6 to reflect the current topology: both producer and consumer are in octo-search-indexer; octo-lib hosts the contract for versioning discipline and cross-repo neutrality, not because two repos need it.
• No changes needed for the current PR to land.

4. Additional Findings

None. The change is minimal, well-scoped, and factually correct on its stated goal.

5. Data Flow Trace

N/A — comment-only change, no runtime data flow affected.

6. Blindspot Checklist (R5)

• C1 双路径 parity: N/A — no symmetric paths in a comment edit.
• C2 control-flow ordering: N/A — no control flow.
• C3 授权边界: N/A — no auth/permission changes.
• C4 授权生命周期: N/A — no auth changes.
• C5 build ≠ runtime: N/A — no build artifacts or runtime paths. go build ./... and go vet ./... pass per PR description.
• C6 治理/策略文档自洽性: N/A — this is a package doc comment, not a governance/policy document.

7. Cross-round Blocker Recheck (R6)

N/A — first review of this PR.

---

Summary: The PR correctly fixes the producer attribution (octo-server → octo-search-indexer), which is verified against the actual codebase. Three P2 notes identify stale framing ("两侧 import", placement rationale, "三仓共享") that predates this PR and is only marginally amplified. No P0/P1 findings.

[Octo-Q] verdict: APPROVE — comment-only change with correct factual fix; P2 notes are pre-existing and non-blocking.

[yujiawei]

Code Review — PR #82 (octo-lib)

Scope: comment-only change to contract/searchmsg/searchmsg.go (3 lines, package doc). No struct, field, SchemaVersion, or logic changes. go build / go vet impact: none (doc comment).

Verdict

APPROVED. The primary fix is correct, and there are no correctness/security/build/functional issues. One documentation accuracy suggestion below (non-blocking) — recommend addressing it in the same PR, since the PR's whole purpose is comment accuracy.

What's correct ✅

• Producer attribution fix is accurate. The producer now lives in the standalone octo-search-indexer repo (internal/producer/*, entrypoint cmd/searchetl-producer), and it imports octo-lib contract/searchmsg. The old wording ("octo-server's searchetl producer") was indeed stale; the new wording reflects reality. Good catch.
• The contract itself is untouched — this stays a pure doc edit as advertised.

Suggestion (P2 — documentation accuracy, non-blocking)

The placement-rationale counterfactual on the same edited block now contradicts the repo topology. The line reads:

把 struct 放在 octo-lib（三仓共享 config 宿主）是硬要求：若放 octo-search-indexer，独立镜像的 es-indexer 无法 import …

But the es-indexer consumer also lives in octo-search-indexer (internal/consumer, cmd/es-indexer, internal/esindex). So if the struct were placed in octo-search-indexer, es-indexer — being in the same repo — could import it without any problem. The counterfactual was mechanically rewritten octo-server → octo-search-indexer alongside the producer fix, which broke its logic: the producer and the consumer now both live in that one repo, so it can no longer be the "outsider that can't import."

The real reason octo-lib must host the contract is the third importer: the read/query path (in octo-server), which is a separate repo from octo-search-indexer. That is what "三仓共享" is pointing at. If the struct lived in octo-search-indexer, the octo-server read path would either be unable to import it cleanly or would have to take a reverse dependency on the indexer repo.

Suggested rewrite of the counterfactual (keep the producer fix as-is, only repair the rationale):

…把 struct 放在 octo-lib（三仓共享 config 宿主）是硬要求：若放 octo-search-indexer，octo-server 侧的 read path（独立仓）就无法干净 import / 会被迫反向依赖 indexer 仓，且会出现「编译过但运行期字段错位静默吃数据」。

This keeps the load-bearing warning intact while making the "who can't import whom" reasoning consistent with the current 3-repo layout. Leaving it as-is risks a future maintainer reading the broken counterfactual and concluding it's safe to move the struct into octo-search-indexer.

Coverage / blind spots

• Reviewed against head SHA; full file read at head to confirm the rest of the package doc (SchemaVersion v2 narrative, safety-field semantics) is unaffected — it is.
• Confirmed no separate es-indexer repo exists (es-indexer ships from octo-search-indexer), which is exactly why the counterfactual is now inaccurate.
• octo-server read-path import of searchmsg was not visible via code search (0 hits) — likely the read path doesn't import the contract yet, or indexing lag. This doesn't affect the verdict but is why the suggested rewrite says "would be unable to import cleanly" rather than asserting a current import edge.

[lml2468]

✅ APPROVE (re-review) — head 441e7cf (was 39f4f75). Both 🔴 blockers resolved.

The author fixed all four sites in one pass — exactly what was asked. Verified on 441e7cf:

1. ✅ Producer attribution is now consistent across the whole package. All four sites agree producer = octo-search-indexer:

• searchmsg.go:3 (searchetl-producer), visibility.go:20, visibility_vectors.go:5, visibility_test.go:10.
  The two remaining octo-server mentions are correct and non-contradictory — they refer to octo-server's reader role (searchmsg.go:4 query-time join filtering revoked/deleted; visibility.go:101 the visiblesAllows parity reference), not the producer. Grepped the full package: no producer attribution conflict remains.

2. ✅ The placement rationale is no longer self-contradictory. The earlier wording put producer + consumer both in octo-search-indexer, which broke the "two different images import it" justification. The new wording fixes the logic correctly: the contract is shared across repos — octo-search-indexer (producer/es-indexer/backfill) and octo-server's read side both import it — so it must live in octo-lib where both repos can import it. That actually answers the architectural question I escalated: searchmsg staying as a standalone package in octo-lib is justified because octo-server's reader genuinely imports it, not just a single image.

Re-verified: go build ./... ok, go vet ./contract/... clean, go test ./contract/searchmsg/ green, gofmt -l clean. Comment-only change; struct/tags/SchemaVersion untouched, wire contract intact.

Verdict: approve — flipping from my prior REQUEST_CHANGES. The attribution is consistent package-wide and the rationale is internally coherent.

Note: head moved 39f4f75 → 441e7cf, so anyone (incl. Steve's REQUEST_CHANGES) on the old head should re-review on 441e7cf to clear the gate.

One thing still worth settling out-of-band (does NOT block this doc PR): the producer-ownership question that drove #81 too. This PR now firmly says producer = octo-search-indexer; #81's commit said "producer retired in prior phases"; #76 said octo-server searchetl. The docs are now self-consistent, but the team should confirm this is the actual, final topology (ideally with the octo-search-indexer producer source as the source of truth) so #81's rationale and this attribution stay in agreement.

[Jerry-Xin]

Summary: This PR is in scope for octo-lib and only updates contract/searchmsg documentation/comments; no contract structs, constants, logic, or tests are changed.

💬 Non-blocking

• 🔵 Suggestion: contract/searchmsg/visibility_vectors.go still says the vectors are shared by “three parties,” while the updated bullets list only the octo-search-indexer producer and backfill paths, and line 7 says “both sides.” Consider making that count consistent.
• 🔵 Suggestion: The PR description says verification showed only a 3-line package-comment change in searchmsg.go, but the checked-out diff changes comments in four files. The implementation is fine, but the PR text should match the submitted diff.

✅ Highlights

• The relevance gate passes: these comments document the exported contract/searchmsg package and its shared visibility contract.
• No behavioral risk found: the submitted diff is comment-only.
• Verification run: go test ./contract/searchmsg passed; git diff --check HEAD~1..HEAD passed.

[mochashanyao]

[Octo-Q · automated review]

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

---

octo-lib PR#82 Review Report

Reviewer: Octo-Q (automated review)
PR: Mininglamp-OSS/octo-lib #82 — docs(searchmsg): fix producer attribution in package comment
Head SHA: 441e7cf
Scope: Comment-only changes across 4 files (+10/-8)

---

1. Verification Summary

Item	Status	Evidence
Factual accuracy of producer attribution	✅	Verified: octo-search-indexer has cmd/searchetl-producer, cmd/es-indexer, cmd/backfill, internal/producer. octo-server/modules/ no longer contains searchetl.
Internal consistency across 4 comment sites	✅	All four files consistently updated from "octo-server searchetl" → "octo-search-indexer". No stale references remain.
No code/struct/field/SchemaVersion changes	✅	Diff is purely comment edits. searchmsg.go struct definitions, visibility.go ExtractVisibility() function, test logic, and test vectors are all untouched.
go build / go vet clean	✅	Per PR body; comment-only changes cannot affect compilation.

2. Findings

P2 — Pre-existing "三方共用" header vs 2-party bullet list (visibility.go:19)

Diff-scope three questions:

1. Pre-existing — not introduced by this PR.
2. The PR changed bullet (1) from "octo-server searchetl producer" to "octo-search-indexer 的 producer" — correct per factual verification. The header "三方共用" was already aspirational (2 current + 1 future reader-side).
3. The header still reads "三方共用（强制 (a) 抽 octo-lib，禁 (b) 各仓重实现，防 #1124 口径分叉）" but only lists 2 current consumers (both now octo-search-indexer) + 1 future ("未来 reader 侧"). After this PR, both current bullets are the same project, making "三方" slightly more misleading than before — but this is a pre-existing structural nit, not a regression.

Severity: P2 (nit). No runtime impact; comment-only. Suggest a follow-up to either update the header to "两方 + 未来 reader" or add the octo-server read-side as a third current consumer (matching the searchmsg.go package comment which does mention "octo-server 读侧").

3. Suggestions

• Consider aligning visibility.go's "三方共用" header with the updated reality: the producer and backfill are both in octo-search-indexer, and octo-server is a read-side consumer. The searchmsg.go package comment already captures this correctly ("octo-search-indexer 镜像的多个命令...以及 octo-server 读侧"). A minor follow-up to make visibility.go consistent with searchmsg.go would be clean.

4. Additional Findings

None. The PR is tightly scoped and accomplishes its stated goal.

5. Data Flow Traceback

N/A — this is a comment-only PR. No data is consumed, produced, or transformed by the diff. The factual claims in the comments were independently verified against the octo-search-indexer and octo-server repository structures.

6. Blind-spot Checklist (R5)

• C1 Dual-path parity: N/A — no symmetric code paths touched.
• C2 Control-flow ordering / nested reuse: N/A — no logic changes.
• C3 Authorization boundary ≠ capability boundary: N/A — no auth/permission changes.
• C4 Authorization lifecycle / container-member state cascade: N/A — no auth changes.
• C5 Build/note ≠ runtime correctness: N/A — no build artifacts, extensions, CLI, or relative URL changes. Comment-only.
• C6 Governance/policy/security doc self-consistency: N/A — this is a package doc comment (code documentation), not a governance/policy document.

7. Cross-round Blocker Recheck (R6)

N/A — first review of this PR.

---

[Octo-Q] verdict: APPROVE — Comment-only PR with verified factual accuracy. One pre-existing P2 nit on "三方共用" header inconsistency, not introduced or amplified by this PR. No P0/P1 findings.

[yujiawei]

Code Review — PR #82 (octo-lib)

Scope reviewed: head SHA 441e7cf3aa8ed29a50bf37403f0b0d852034bfc3, comment-only changes under contract/searchmsg/.

Summary

This PR corrects a stale attribution in the searchmsg package documentation: it stops describing the Kafka producer as octo-server's built-in searchetl and re-points it to the standalone octo-search-indexer image. The core correction is accurate and well-targeted, verified against the actual code in the producer/consumer repos (not taken on the PR's word). The change is comment-only, builds and vets cleanly, and carries zero runtime/wire/schema risk.

I'm approving it. There is one new doc inaccuracy and one PR-body scope mismatch worth fixing — both non-blocking (P2).

1. Verification of the core claim — ✅ correct

• octo-server no longer hosts the producer. A repo-wide search of octo-server finds no reference to searchmsg, contract/searchmsg, or any Kafka producer for octo.message.v1; go.mod carries no Kafka client. The only surviving searchetl token is modules/base/sql/20260614000001_searchetl_init.sql, whose own header documents that the formerly-built-in modules/searchetl was removed and the cursor table handed to the standalone producer. octo-server's remaining search modules are read/query-side only.
• octo-search-indexer hosts the producer. cmd/searchetl-producer + internal/producer build searchmsg.Message and write to octo.message.v1 via kafka.Writer.WriteMessages. The visibility reattribution is likewise correct: real-time enrichment (internal/producer/extract.go) and backfill enrichment (internal/backfill/doc_enrich.go) both call searchmsg.ExtractVisibility and fail-closed to DLQ, and both consume the single shared FailClosedVisibilityVectors() set defined in octo-lib. No divergent copy exists in either downstream repo.

So the old wording was genuinely wrong, and the new wording matches reality. Good catch.

2. Findings

P2 — A new attribution inaccuracy is introduced in the same comment

contract/searchmsg/searchmsg.go now opens with (present tense):

该契约跨仓共同 import：octo-search-indexer 镜像的多个命令（…），以及 octo-server 读侧（query-time join 过滤 revoked/deleted）与共享单测向量。

This asserts that octo-server is a current cross-repo importer of the contract package and a co-consumer of the shared test vectors. That is not true of the code today:

• octo-server has zero references to contract/searchmsg, ExtractVisibility, or FailClosedVisibilityVectors.
• octo-server's go.mod pins an octo-lib revision predating this PR, with no replace directive and no vendored copy.
• octo-server does filter revoked/deleted at query time, but via its own independent Elasticsearch DSL (MustNot(term revoked=true)) — not by importing this contract.

For a PR whose entire purpose is accuracy of attribution, it's worth not replacing one inaccurate claim with another. Suggested fix: phrase the octo-server leg as intended/planned rather than current (matching the pre-existing "未来 reader 侧若需从 payload 复核可见性" framing already in visibility.go), or drop the "与共享单测向量" co-consumption clause for octo-server. Non-blocking.

P2 — PR body understates its own scope

The description says the diff "contains only the package comment in contract/searchmsg/searchmsg.go (3 lines)." The actual diff touches 4 files (searchmsg.go, visibility.go, visibility_test.go, visibility_vectors.go), +10/-8. "Comment-only" is accurate; "one file, 3 lines" is not. A reviewer relying on the body alone would under-review three of the four touched files. Please align the body with the diff.

3. Suggestions

1. Soften or correct the octo-server leg in the searchmsg.go package comment so it doesn't assert a cross-repo import / shared-vector consumption that doesn't exist yet.
2. Update the PR body's scope description to "4 files, comment-only" so future readers and bisectors aren't misled.

4. Additional notes (out of scope, no action required)

• config/config.go references to es-indexer are correct (they name the OpenSearch-writer binary) and rightly left untouched.
• I verified absence of producer code in octo-server, not deployment/runtime wiring; that's outside a doc-only PR's remit.

Verdict

No P0/P1 issues. Comment-only, builds clean, core correction is right. Approve, with the two P2 doc-hygiene items above recommended (not required) before or after merge.