mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-14 08:18:19 +00:00
fix(pancake): demote hot-path webhook log to Debug; guard commentID before echo (#916)
- webhook_handler.go: demote per-event "page_id resolution" log from Info to Debug. Firing on every webhook was the exact anti-pattern called out in the routing-metadata refactor review. - pancake.go: move reply_to_comment_id guard above rememberOutboundEcho so a missing-metadata error does not stamp phantom echoes that would pollute inbound echo dedup. - docs/journals: drop three Pancake journals (not needed in repo).
This commit is contained in:
Viet Tran authored and GitHub committed
1 parent
85e6f216b5
commit
60ca45ec97
5 files changed
+11
-210
No files matched your search
@@ -1,85 +0,0 @@
|
||||
# Pancake Platform: Mandatory Dropdown + Form Validation Gap Fix
|
||||
|
||||
**Date**: 2026-04-14 09:00
|
||||
**Severity**: Medium
|
||||
**Component**: `ui/web/src/pages/channels/`, `internal/channels/pancake/`
|
||||
**Status**: Resolved
|
||||
|
||||
## What Happened
|
||||
|
||||
Pancake channel creation let operators leave the `platform` field blank or type a freeform string. Auto-detection from webhook payloads was the supposed fallback — but auto-detection is best-effort and silent-wrong when it fires on an ambiguous payload. An operator could create a Pancake channel, send messages through it for days, and only discover the platform was misidentified when behavior diverged between Facebook and Instagram routing.
|
||||
|
||||
Separately, the channel form had no pre-submit validation on `required` config fields. The schema marked fields as `required: true` but the form dialog never checked — it shipped empty strings to the backend and let the backend fail.
|
||||
|
||||
Both were fixed: platform field is now a required dropdown with 11 explicit options, and the form dialog enforces `required` config fields on submit (create path only, to avoid breaking legacy channels with no platform stored in DB).
|
||||
|
||||
## The Brutal Truth
|
||||
|
||||
The "auto-detect as fallback" design was wishful thinking. Auto-detection works when every webhook payload has a reliable, consistent platform discriminator. It doesn't work when platform discrimination is inferred from message shape rather than an explicit field. Shipping a form with an optional platform and crossing fingers on runtime detection was a correctness gamble that never needed to be made — the user knows which platform they're configuring at creation time.
|
||||
|
||||
The `required` validation gap is more embarrassing. The schema system had a `required` property on field descriptors, but the form dialog never read it before submitting. The property was there for documentation purposes only. This is the worst kind of dead code: it looks like it works, reviewers assume it works, and it silently fails to enforce anything.
|
||||
|
||||
## Technical Details
|
||||
|
||||
Schema change in `channel-schemas.ts`:
|
||||
|
||||
```ts
|
||||
// Before
|
||||
{ key: "platform", type: "text", required: false, hint: "Auto-detected if empty" }
|
||||
|
||||
// After
|
||||
{ key: "platform", type: "select", required: true, defaultValue: "",
|
||||
options: [
|
||||
{ value: "facebook", label: "Facebook" },
|
||||
{ value: "instagram", label: "Instagram" },
|
||||
// ... 9 more
|
||||
] }
|
||||
```
|
||||
|
||||
Validation block added in `channel-instance-form-dialog.tsx`, scoped to create-only:
|
||||
|
||||
```ts
|
||||
if (!isEditing) {
|
||||
const missing = schema.configFields
|
||||
.filter(f => f.required && !cleanConfig[f.key])
|
||||
.map(f => f.key);
|
||||
if (missing.length > 0) {
|
||||
setErrors({ config: missing });
|
||||
return;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Validated against `cleanConfig` (post empty-string strip), not `configValues` — catches both `undefined` and `""` from an unselected dropdown. Validating `configValues` would have missed the `""` case because `cleanConfig` strips empty strings before the check.
|
||||
|
||||
Backend: `pancake.go` auto-detect log demoted from `slog.Info` to `slog.Debug`. The previous level would have spammed logs for every legacy channel restart indefinitely.
|
||||
|
||||
Test run: 22 UI tests pass (`channel-schemas.test.ts` + pre-existing suite). Go pancake tests pass. PG and SQLite builds clean.
|
||||
|
||||
## What We Tried
|
||||
|
||||
No dead ends. TDD approach: wrote `channel-schemas.test.ts` with 5 failing assertions (Red), implemented the schema change, confirmed Green (22/22). The test-first pass caught a defaultValue omission before it would have caused a subtle unset-state bug in the dropdown.
|
||||
|
||||
## Root Cause Analysis
|
||||
|
||||
Two independent oversights:
|
||||
|
||||
1. Platform field: schema author assumed auto-detection was robust enough to make explicit selection optional. It isn't. Explicit always beats inferred for configuration that has deterministic user knowledge at creation time.
|
||||
|
||||
2. Required validation: the `required` property was added to the schema descriptor type without a corresponding enforcement pass in the form dialog. Classic schema-as-documentation antipattern — the property communicates intent but enforces nothing. No test existed to verify that submitting a blank required field was actually blocked.
|
||||
|
||||
## Lessons Learned
|
||||
|
||||
1. **If the user knows it at creation time, make them set it.** "Auto-detect as fallback" is only valid when the value genuinely cannot be known upfront. Platform is a deliberate operator choice, not a runtime observable.
|
||||
|
||||
2. **Schema properties that imply behavior must be enforced.** `required: true` in a field descriptor is a promise to the operator. If the form doesn't read it, the promise is a lie. Any time a new semantic property is added to a schema type, ask immediately: "where does this actually get enforced?"
|
||||
|
||||
3. **Validate `cleanConfig`, not `configValues`.** The form strips empty strings from config before submission. Validating before the strip means `""` from an unselected dropdown bypasses the required check. Validate the value in the same form it will be submitted.
|
||||
|
||||
4. **Edit-path must not block legacy data.** Existing Pancake channels in production have no `platform` stored — they relied entirely on auto-detection. Making `platform` required on edit would lock operators out of editing those channels until they backfill the field. Create-only enforcement + runtime auto-detect fallback is the right seam.
|
||||
|
||||
## Next Steps
|
||||
|
||||
- [ ] Confirm exact Pancake API keys for `threads`, `youtube`, `google`, `chat_plugin` against live Pancake docs or a real webhook payload. Current values are best-guess from naming convention — if any are wrong, routing will silently mismatch. Owner: whoever next handles a Pancake integration with those platforms.
|
||||
- [ ] Backfill migration or UI prompt for existing Pancake channels with no `platform` stored — auto-detect will cover them at runtime but operators have no visibility into which platform was inferred. Low urgency, high debuggability value.
|
||||
- [ ] Extend required-field validation to the edit path once legacy channels have been audited. Right now edit path has no guard. Owner: channels team, after confirming no production channel has an unset required field.
|
||||
@@ -1,75 +0,0 @@
|
||||
# Routing Metadata Refactoring + Pancake Comment Diagnostics
|
||||
|
||||
**Date**: 2026-04-14 03:00
|
||||
**Severity**: High
|
||||
**Component**: `internal/channels`, `internal/channels/pancake`, `cmd/gateway_consumer_normal.go`
|
||||
**Status**: Resolved
|
||||
|
||||
## What Happened
|
||||
|
||||
Pancake page comment replies were landing in inbox instead of the public comment thread. The inbound handler correctly stamped `pancake_mode=comment` onto message metadata, but outbound delivery discarded that key before it reached `Send()`. Every comment reply quietly defaulted to inbox routing. No error, no log, no crash — just silent wrong behavior that required a live test to detect.
|
||||
|
||||
The fix cascaded into a proper structural refactor: extracted all routing key definitions and copy helpers into `internal/channels/routing_metadata.go`, added `pancake_mode` to the canonical key set, switched `gateway_consumer_normal.go` to use `channels.CopyFinalRoutingMeta()` instead of its own private inline loop, and wrapped the feature-disabled log in `sync.Once` to prevent spam. 16 new comment handler tests and 2 routing_metadata unit tests were added. All 77 channels package tests and 64 pancake package tests pass.
|
||||
|
||||
## The Brutal Truth
|
||||
|
||||
This was not subtle. The metadata key list in `cmd/gateway_consumer_normal.go` and the one in `internal/channels/events.go` had already diverged before `pancake_mode` was ever needed. We had two separate hardcoded string slices doing the same job in different files, neither derived from the other. Adding a new routing key meant updating two places, and we missed one. Classic DRY violation with delayed detonation.
|
||||
|
||||
The maddening part: the bug was completely silent. No test caught it because there were no tests for metadata preservation across the inbound→outbound hop. The only signal was a live comment reply going to the wrong place. We were testing whether messages arrived, not whether they arrived *correctly routed*.
|
||||
|
||||
The `sync.Once` diagnostic fix is also a minor embarrassment. The original `slog.Info` on every disabled comment event would have hammered logs the moment any page with `comment_reply=false` received traffic. It should have been gated from the start. `sync.Once` is the minimal fix, but the real lesson is: always ask "how often will this fire in production?" before adding a per-event log inside a hot path.
|
||||
|
||||
## Technical Details
|
||||
|
||||
Root cause path:
|
||||
|
||||
1. `pancake/comment_handler.go` sets `metadata["pancake_mode"] = "comment"` (line 77)
|
||||
2. `cmd/gateway_consumer_normal.go` line 218: `outMeta := channels.CopyFinalRoutingMeta(msg.Metadata)` — **before the fix** this was an inline loop over a hardcoded key slice that did not include `pancake_mode`
|
||||
3. `outMeta` is passed into `ChannelMgr.RegisterRun()` and then forwarded to all outbound publishes
|
||||
4. `pancake/pancake.go` `Send()` checks `msg.Metadata["pancake_mode"]` to branch between comment reply API and inbox reply API — empty key → inbox branch
|
||||
|
||||
The surviving inline loop in `events.go` block.reply path (lines 271-275) still reads directly from `routingMetaKeys` (now the canonical exported slice), which is technically correct but still not using the `copyRoutingMeta()` helper. Code review flagged this as informational — non-blocking because block replies intentionally exclude `placeholder_key` and the keys read are identical to what `copyRoutingMeta()` would copy. But it is residual inconsistency.
|
||||
|
||||
`finalReplyMetaKeys` design:
|
||||
|
||||
```go
|
||||
var finalReplyMetaKeys = append([]string{
|
||||
"placeholder_key", // final outbound can update placeholder; block replies must not
|
||||
}, routingMetaKeys...)
|
||||
```
|
||||
|
||||
`placeholder_key` is in `finalReplyMetaKeys` only. Block replies use `routingMetaKeys` (no `placeholder_key`). If a block reply carried `placeholder_key`, the placeholder message would be overwritten mid-stream before the final response was ready. Keeping the two lists distinct was the right call.
|
||||
|
||||
Test counts: `go test ./internal/channels/... -count=1` → 77 pass, 0 fail. `go test ./internal/channels/pancake/... -count=1` → 64 pass, 0 fail.
|
||||
|
||||
Code review findings (all informational, non-blocking):
|
||||
- `events.go` block.reply path still uses inline loop — not using `copyRoutingMeta()` helper
|
||||
- `routing_metadata_test.go` missing nil-map input test for `copySelectedMeta`
|
||||
- `sync.Once` on `commentReplyDisabledOnce` cannot be reset between tests — `sync.Once` reinit path untested
|
||||
|
||||
## What We Tried
|
||||
|
||||
No dead ends this session. The root cause was identified immediately from the previous investigation journal (`260414-0225-pancake-comment-routing-metadata-fix.md`). This session was the structural cleanup and test coverage pass that followed the point-fix.
|
||||
|
||||
## Root Cause Analysis
|
||||
|
||||
Two metadata preservation code paths were maintained independently with no shared contract. When `pancake_mode` was added as a routing key in the pancake channel, it was added to the inbound metadata map but there was no mechanism to enforce that all outbound copy paths would automatically include it. The lack of a canonical routing key registry meant every place that copied metadata had to be manually updated — and one wasn't.
|
||||
|
||||
Secondary cause: zero tests existed for the metadata→outbound preservation hop. The inbound handler was tested (produces metadata), the outbound channel was tested (accepts metadata), but the pipeline connecting them was not. The gap was invisible until production routing broke.
|
||||
|
||||
## Lessons Learned
|
||||
|
||||
1. **Routing keys must have one source of truth.** Any code that copies a subset of metadata for routing must derive from a shared registry. Ad-hoc key lists in consumer code are a time bomb. `routing_metadata.go` is that registry now — it must stay the only place key lists live.
|
||||
|
||||
2. **Test the pipeline seam, not just the endpoints.** Testing that inbound sets a metadata key and testing that outbound reads a metadata key is not the same as testing that the key survives the round trip. The round-trip test (`TestHandleCommentEvent_FeatureEnabled` checking `msg.Metadata["pancake_mode"] == "comment"`) is the test that would have caught this bug before it shipped.
|
||||
|
||||
3. **Per-event logs in hot paths need rate controls from day one.** `slog.Info` inside a webhook event handler that fires on every Facebook comment is a logging DoS waiting to happen. If a diagnostic log is informational and fires on a repeatable condition (feature disabled), use `sync.Once` or a rate limiter. Don't retrofit it after the fact.
|
||||
|
||||
4. **Separation of `placeholder_key` from routing keys is a load-bearing design choice.** If this distinction is ever collapsed or "simplified," block replies will corrupt placeholders mid-stream. Future developer: do not merge `finalReplyMetaKeys` and `routingMetaKeys` without understanding this.
|
||||
|
||||
## Next Steps
|
||||
|
||||
- [ ] Refactor `events.go` block.reply path to call `copyRoutingMeta()` instead of the remaining inline loop — eliminates the last inconsistency flagged in code review. Owner: any engineer touching `events.go` next. No urgency, non-blocking.
|
||||
- [ ] Add nil-map safety test for `copySelectedMeta(nil, keys)` in `routing_metadata_test.go` — current implementation calls `src[k]` on nil map which panics in Go. Owner: next person adding routing metadata tests. **This is a latent panic, not just a missing test.**
|
||||
- [ ] Add outbound Pancake action log (`reply_comment` vs `reply_inbox`) so operators have runtime proof of which branch fires without needing to read source code. Owner: pancake channel work.
|
||||
- [ ] Evaluate whether all channel-specific routing keys should be defined in one canonical schema to prevent the same drift happening for future channels. This is architectural, needs team discussion before acting.
|
||||
@@ -1,39 +0,0 @@
|
||||
# 2026-04-12 Pancake Comment Auto-Reply + First Inbox
|
||||
|
||||
## Summary
|
||||
|
||||
Extended Pancake (pages.fm) channel to handle comment-on-post conversations and first-inbox auto-replies in parallel with existing inbox flows. Implemented 5 phases: types/API client, post fetcher with singleflight, comment filtering/routing, webhook router split, and send path refactor. All flows remain isolated; zero impact on inbox stability.
|
||||
|
||||
## Key Changes
|
||||
|
||||
- **Types:** Added `Features.FirstInbox`, `CommentReplyOptions` (filter/keywords/includePostContext), `PostID` to conversations, `PancakePost` type
|
||||
- **API Client:** New `ReplyComment()`, `PrivateReply()`, `GetPosts()` methods with action-based routing
|
||||
- **Post Fetcher:** New `post_fetcher.go` with `sync.Map` cache + `singleflight` dedup (key: `"posts"` because Pancake requires list-all, not per-post lookup)
|
||||
- **Comment Handler:** New `comment_handler.go` with feature gate, self-reply prevention, staff skip, dedup by `comment:{msgID}`, keyword/all filter, echo dedup, post context enrichment
|
||||
- **Webhook Router:** `webhook_handler.go` split `if convType != "INBOX"` into `switch` cases for INBOX vs COMMENT with PostID normalization
|
||||
- **Send Path:** `pancake.go` refactored `Send()` into `sendInboxReply()` / `sendCommentReply()` based on `metadata["pancake_mode"]`; added `sendFirstInbox()` with `sync.Map` dedup
|
||||
|
||||
## Design Decisions
|
||||
|
||||
- **ChatID for comments = ConversationID:** Pancake groups comment conversations per sender per post internally (unlike Facebook where we constructed `postID:senderID` ourselves)
|
||||
- **sendFirstInbox error handling:** Delete `firstInboxSent` entry on error to allow retry, not the stored time. Allows operator to re-trigger without waiting for TTL
|
||||
- **Singleflight key scope:** All posts fetched with key `"posts"` (not per-postID) because Pancake API requires `GetPosts()` list-all, not per-post lookup. Prevents API stampede on concurrent comment arrivals for different posts
|
||||
- **Additive architecture:** INBOX flow completely unchanged. Comment flow runs parallel. Feature gate ensures zero blast radius
|
||||
|
||||
## Test Coverage
|
||||
|
||||
- 72 tests pass with `-race` (integration + unit)
|
||||
- Both `go build ./...` (PG) and `go build -tags sqliteonly ./...` (SQLite) clean
|
||||
- `go vet ./...` clean
|
||||
- Post fetcher, comment handler, send path all tested with dedup/concurrency/negative cache scenarios
|
||||
|
||||
## Follow-up Fixes
|
||||
|
||||
Code review fixes applied (PR #841 feedback from @mrgoonie):
|
||||
- Added `slog.Debug` on post context fetch failure in `buildCommentContent()`
|
||||
- Documented `GetPosts()` bypass of `doRequest()` (response body parsing requirement)
|
||||
- Added 72h TTL eviction in `runDedupCleaner()` for `firstInboxSent` sync.Map (prevents unbounded growth)
|
||||
- Wrapped `sendCommentReply()` with `context.WithTimeout(30s)` (bounds API hang risk)
|
||||
- Replaced custom `containsStr` test helper with `strings.Contains`
|
||||
|
||||
All non-blocking, no architecture changes.
|
||||
@@ -254,18 +254,20 @@ func (ch *Channel) sendCommentReply(ctx context.Context, msg bus.OutboundMessage
|
||||
defer cancel()
|
||||
|
||||
conversationID := msg.ChatID
|
||||
text := FormatOutbound(msg.Content, ch.platform)
|
||||
|
||||
// Remember echo before sending (same pattern as inbox).
|
||||
// Guard first — otherwise rememberOutboundEcho would stamp phantom echoes
|
||||
// for a send that never happens, polluting future inbound echo dedup.
|
||||
commentID := msg.Metadata["reply_to_comment_id"]
|
||||
if commentID == "" {
|
||||
return fmt.Errorf("pancake: reply_to_comment_id missing in outbound metadata for comment reply")
|
||||
}
|
||||
|
||||
text := FormatOutbound(msg.Content, ch.platform)
|
||||
parts := splitMessage(text, ch.maxMessageLength())
|
||||
for _, part := range parts {
|
||||
ch.rememberOutboundEcho(conversationID, part)
|
||||
}
|
||||
|
||||
commentID := msg.Metadata["reply_to_comment_id"]
|
||||
if commentID == "" {
|
||||
return fmt.Errorf("pancake: reply_to_comment_id missing in outbound metadata for comment reply")
|
||||
}
|
||||
for _, part := range parts {
|
||||
if err := ch.apiClient.ReplyComment(ctx, conversationID, commentID, part); err != nil {
|
||||
ch.handleAPIError(err)
|
||||
|
||||
@@ -142,12 +142,10 @@ func (r *webhookRouter) ServeHTTP(w http.ResponseWriter, req *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
// INFO-level: always visible without DEBUG — shows which field sourced the page_id.
|
||||
slog.Info("pancake: webhook page_id resolution",
|
||||
"event_page_id", event.PageID,
|
||||
"data_page_id", data.PageID,
|
||||
"conv_id", data.Conversation.ID,
|
||||
slog.Debug("pancake: webhook event parsed",
|
||||
"event_type", event.EventType,
|
||||
"resolved_page_id", pageID,
|
||||
"conv_id", data.Conversation.ID,
|
||||
"conv_type", convType,
|
||||
"sender_id", data.Conversation.From.ID,
|
||||
"msg_id", data.Message.ID)
|
||||
|
||||
Reference in new issue
Block a user