Code Review

Great cleanup — net -570 LOC across 33 modules is meaningful, and the consolidations are mostly well-chosen. The OTLP unification, KQL operator table, and golden-cache helper are all solid abstractions. A few things worth addressing before merge:

Correctness / Bugs

1. scrubNulText changes behavior: replaces \NUL with U+FFFD instead of deleting
src/Utils.hs / src/Models/Telemetry/Telemetry.hs

The old cleanNullBytes used T.filter (/= '\NUL') (deletion). The new scrubNulText uses T.replace "\NUL" "\xFFFD". U+FFFD replacement characters will now appear in stored attribute values, log previews, and query results — a visible change for any consumer that expected clean strings. Additionally, scrubNulValue also scrubs object keys (via scrubNulText (AEK.toText k)) which the old cleanNullBytesFromJSON did not do. If the replacement-character behavior is intentional, it should be called out explicitly; if not, revert to filtering.

2. Unused firstPart binding in recordProtoError
src/Opentelemetry/OtlpServer.hs

After inlining createProtoErrorInfo, the pattern (firstPart : details) binds firstPart but never references it — only details is used. Should be (_ : details). This was latent in the deleted helper but the refactor re-exposes it and will trigger a -Wall warning.

3. error "unreachable" wildcards lose exhaustiveness checking
src/Pkg/Parser/Expr.hs — Display Expr and ToQueryText Expr instances

The subBinaryParts / valBinaryParts lookup-then-find pattern is fine for existing constructors, but the fallthrough wildcard _ -> Nothing mapping to error "unreachable" means GHC can no longer warn if a new Expr constructor is added without updating these functions — it becomes a silent runtime crash instead of a compile-time exhaustiveness error. Consider pattern-matching exhaustively on the Expr constructors (moving the table lookup into each arm) or at least adding a {-# WARNING #-} pragma.

Potential Issues

4. processSignalRequest still calls projectIdsByProjectApiKeys twice
src/Opentelemetry/OtlpServer.hs

Pre-existing, not introduced here, but the refactor was a natural opportunity to fix it: the function calls projectIdsByProjectApiKeys inside the when guard and again unconditionally a few lines later. The second call is redundant — the result from the first could be threaded through.

5. GenericLLMCore.Message standalone deriving is fragile
src/Data/Effectful/LLM.hs

deriving stock instance Generic LLMCore.Message will fail with a duplicate-instance error if langchain-hs ever adds its own Generic derivation to Message. The comment says "record field names match the prior hand-written keys" — this should be verified against the library's actual field names, since ToJSON/FromJSON via Generic will use the Haskell field names, not custom fieldLabelModifier.

6. toUriStr equivalence to urlEncodeText unverified
src/Web/MCP.hs

The original used decodeUtf8 . H.urlEncode True . encodeUtf8 (component encoding, spaces → %20). If toUriStr in Utils uses H.urlEncode False (query encoding, spaces → +), this silently changes URL output. Worth a quick grep/verify.

Style / Minor

7. billingNotifEmail weakens heading type from Text to Html ()
src/Pkg/EmailTemplates.hs — minor loss of precision; h1_ [heading] wrapped in Html () where Text would let the type system reject non-text headings.

8. dashboardSelectBlock placeholder text is wrong for widget-select
src/Pages/Bots/Slack.hs — Pre-existing bug carried through: placeholder still reads "Select a dashboard template" even when used as a widget selector. Not introduced here, but worth fixing while the code is being touched.

9. setTimestampColByIds name is too implementation-specific
src/Models/Apis/Monitors.hs — The shared helper is really just \f -> Time.currentTime >>= Hasql.interpExecute . f. A more general name (withCurrentTime or similar) would serve any future callers without confusion.

What's working well

• OTLP processSignalRequest correctly handles all cases from both original handlers; the type parameter approach is clean.
• KQL binary operator table (subjectBinOps / valBinOps) correctly preserves parser precedence — the find-by-parse-symbol approach is safe for reordering.
• pValuesWith self extra recursion correctly threads self so pValuesNoFunc still disallows scalar functions inside nested lists.
• atMapInt switching from round to toBoundedInteger is strictly more correct (avoids silent overflow).
• WrappedEnumInt via DerivingVia is a nice use of the extension for repetitive enum boilerplate.
• withGoldenCache abstraction is well-scoped and the HTTP exception handling is correctly preserved in the callers.

Overall: items 1–3 should be addressed before merge. Items 4–6 are worth a quick check. The rest are minor.

Code Review - PR #431: consolidation/LOC reduction

Overall this is well-executed. The structural patterns (withGoldenCache, WrappedEnumInt, processSignalRequest, mkOtlpRpcHandler, binary-op tables, buildJudgePrompt, selectOtelSpans, BulkOpResult) are all solid uses of the codebase's existing machinery. A few issues to address before merging:

Bugs / Behaviour Changes

1. scrubNulText now strips NUL instead of replacing with U+FFFD (src/Utils.hs)

The old behaviour preserved string length (substituted visible replacement char U+FFFD); the new one removes the byte entirely. Callers that previously showed a replacement glyph for NUL in trace attributes get a shorter string. The docstring was updated to say "Strip" - confirm this is intentional rather than a copy-paste from the local cleanNullBytes helper that used filter.

2. Display Expr / ToQueryText Expr lose compile-time exhaustiveness (src/Pkg/Parser/Expr.hs)

The previous hand-enumerated pattern matches gave a GHC exhaustiveness warning if a new Expr constructor was added without updating Display/ToQueryText. The new fall-through with error "Display Expr: unreachable" silently compiles and crashes at runtime. The subBinaryParts/valBinaryParts lookup tables are separate from the constructor definition, so it is easy to add a constructor and forget a table row. This is the one place in the PR where the abstraction trades compile-time safety for brevity - worth considering a total helper or at least a NOTE comment near the tables pointing to the instances.

3. dashboardSelectBlock adds "Untitled" guard to widget names (src/Pages/Bots/Slack.hs)

The merged builder applies if T.null text then "Untitled" else text to every option. The old dashboardViewTwo (widget selector) did NOT have this guard - widgets with an empty title previously showed blank; they now show "Untitled" in the Slack modal. Minor UI change; confirm it is intentional.

4. getRequestDetails uses Map.member instead of pattern-matching on AE.Object (src/Pages/LogExplorer/LogItem.hs)

Old code only entered the HTTP branch when Map.lookup "http" m yielded Just (AE.Object _). New code enters whenever the key exists regardless of value shape. If m["http"] is a String or Null, the old code fell through to check rpc/db; the new code returns Just ("HTTP", "", 0). Unlikely in practice but a latent difference.

5. FromField (Map Text AE.Value) - verify AesonText covers the varchar fallback (src/Models/Telemetry/Telemetry.hs)

Old code tried JSONB first, then a text/varchar parse as fallback (tryJsonb <|> tryText). New code delegates entirely to AesonText's FromField via coerce. If AesonText's fromField only handles JSONB and not varchar/text, the fallback path is silently dropped. Worth confirming DeriveUtils.AesonText handles both.

Minor style notes

getRequestDetails triple-nested ifs - less readable than the prior case chain. asum with guard or <|> would be more idiomatic and remove a nesting level.

processSignalRequest positional Text args - 4 consecutive Text parameters (label, signal, receivedMsg, noun) are easy to transpose; a small record would make future edits safer. Not a blocker.

unAcknowlegeGet route binding (src/Web/Routes.hs) - grew a lambda because the merged handler added Maybe Text _hostM that unAcknowledgeAnomalyGetH did not have. Since _hostM is ignored on the False path, dropping it from the merged signature would remove the lambda at the call site.

What is working well

• WrappedEnumInt via-deriving is the right pattern for AggregationTemporality and any future int-enum columns.
• withGoldenCache cleanly eliminates three near-identical 30-line blocks.
• mkOtlpRpcHandler and processSignalRequest correctly unify trace/log/metrics pipelines without breaking per-signal differences.
• buildJudgePrompt parametrisation of the three LLM judge prompts is clean.
• BulkOpResult/BulkSucceeded is a genuine API improvement - partial-success is now expressible without the Set workaround apiLogPatternsBulk was using.
• selectOtelSpans, binary-op tables, dashboardsByProjectJoin, setTimestampColByIds, sortableTableHead_, and widgetFetchUrl are all good data-driven consolidations.
• lsGet + LSData generalises the two Lemon Squeezy fetch helpers cleanly via deriving via DAE.CustomJSON.
• Generic-derived LLMCore.Message JSON is byte-identical to the prior hand-written instances (record field names match).

Summary: Items 2 (exhaustiveness loss in Expr instances) and 5 (AesonText varchar fallback) are the highest-priority checks. Items 1, 3, and 4 are genuine behaviour changes that should be deliberate. Everything else is optional polish.

Code Review — refactor: consolidate duplicated code to reduce LOC

Overall this is a well-structured, well-documented refactor. The LOC reduction is real, the split into three phases was the right call, and the test/doctest coverage is solid. A few issues worth addressing before merge:

Issues

1. scrubNulText — silent behavior change (not behavior-preserving)

-- Old: replaces \NUL with U+FFFD (replacement char)
scrubNulText t = if T.any (== '\NUL') t then T.replace "\NUL" "\xFFFD" t else t
-- New: strips \NUL entirely
scrubNulText t = if T.any (== '\NUL') t then T.filter (/= '\NUL') t else t

The PR description claims "behavior-preserving consolidation" but this is a semantic change: the old path turned "foo\NULbar" into "foo\xFFFDbar" (13 chars); the new path yields "foobar" (6 chars). If any downstream consumer (JSON parsers, span field comparisons) treats U+FFFD as a sentinel for malformed data, this will silently change behavior. The new approach is arguably simpler, but it's not behavior-preserving and should be explicitly called out.

2. Display Expr / ToQueryText Expr — exhaustiveness replaced by error

-- New
displayPrec _ _ = error "Display Expr: unreachable"
toQText _ = error "ToQueryText Expr: unreachable"

The old exhaustive match gave a compile-time guarantee that every Expr constructor was handled. The new table-driven approach silently compiles when a new constructor is added but crashes at runtime. The code comment acknowledges this risk, but the right fix is to preserve the compile-time guarantee — use a wildcard that pattern-matches the known "handled" constructors explicitly, or keep the exhaustive match for the non-table cases. Turning a compiler error into a runtime panic is a type-safety regression.

3. WrappedEnumInt — inconsistent error handling between FromField and HI.DecodeValue

instance (Bounded a, Enum a, Typeable a) => FromField (WrappedEnumInt a) where
  fromField f bs = fromField @Int f bs
    >>= \n -> maybe (returnError ConversionFailed f ...) (pure . WrappedEnumInt) (safeToEnum n)

instance (Bounded a, Enum a) => HI.DecodeValue (WrappedEnumInt a) where
  decodeValue = WrappedEnumInt . fromMaybe minBound . safeToEnum <$> HI.decodeValue

FromField correctly fails on an out-of-range int. HI.DecodeValue silently maps any invalid value to minBound. This is a silent data corruption risk for Hasql decode paths — an invalid DB value will appear as the zero-enum without any error. Use refineText/explicit fail on Nothing instead of fromMaybe minBound.

4. subBinaryParts / valBinaryParts — intermediate string key is fragile

subBinaryParts e = do
  (s, v, sym) <- case e of
    Eq s v -> at s v "=="
    ...
  (_, _, qtok, dop) <- find (\(_, sym', _, _) -> sym' == sym) subjectBinOps
  pure (s, v, dop, qtok)

This does two passes: match constructor → symbol string, then look up that symbol in the table. A typo in any symbol string compiles cleanly but fails silently (returns Nothing instead of the right token). The simpler alternative is to find the constructor directly, or return the table row index from the first match instead of the intermediate symbol string. Also, find on a list is O(n) — not a problem at this size, but a Map Text (Text, Text) keyed by the parse symbol would be both faster and eliminate the fragile string coupling.

Minor Notes

5. processSignalRequest — 8 positional Text/AE.Key params

processSignalRequest label signal receivedMsg noun countKey metadataApiKey projectKeys atIds' convert

Eight positional arguments of the same type are easy to mix up at call sites. A small ProcessSignalConfig record would make the two call sites (processTraceRequest, processLogsRequest) self-documenting with no LOC cost since both callers are in the same file.

6. LLMCore.Message — Generic deriving on a foreign type

deriving stock instance Generic LLMCore.Message
deriving anyclass instance AE.ToJSON LLMCore.Message
deriving anyclass instance AE.FromJSON LLMCore.Message

The PR notes byte-identical semantics — this holds only if LLMCore.Message's record field names are exactly role, content, messageData with no Generic customisation. If the upstream library changes field names or adds fields, the derived instances silently diverge from the old manual ones. Worth a one-time doctest asserting the round-trip of a known Message value to lock this in.

7. runKqlWithRawData callers pass AE.Null as raw data

(.formatted) <$> runKqlWithRawData config kql [] (\(results,_,_) -> (formattedText, AE.Null))

Four callers do this. The runKqlAndFormat wrapper being removed was precisely the abstraction that avoided surfacing the AE.Null dummy. A runKqlFormatOnly wrapper (one line) would keep call sites clean without re-introducing duplication.

What's good

• withGoldenCache unification is clean; the HTTP exception handling is correctly preserved in the getOrCreateGoldenResponse adapter.
• border / takeFirst / validTsOr / resourceAttributeValue consolidations are all strictly better.
• pValuesWith sharing pValues/pValuesNoFunc without behaviour change is elegant.
• formatUTCMicros extraction eliminates a copy-pasted format string across several modules.
• WrappedEnumInt derivation and enumSCValues helper are well-designed.
• mkOtlpRpcHandler type constraints are verbose but unavoidable given the gRPC typeclass hierarchy.