Code Review

Overview

Large, well-motivated refactor that ships three distinct improvements together: (1) agentic-CLI hardening (stable JSON envelopes, facets command, input validation, --debug, cursor pagination), (2) server-side DRY-up (mkPageCtx, toastError, withCardFrame), and (3) deriving-aeson / typed-struct adoption. The direction is good and the line-count reduction is real. A few issues worth resolving before merge:

Issues

1. Breaking env-var rename with no migration path
MONO_API_KEY → MONOSCOPE_API_KEY (and MONO_PROJECT, MONO_API_URL, MONO_AGENT_MODE) is a breaking change for every existing user/CI pipeline. The docs note it but there's no backward-compat shim (even a deprecation notice that reads both). If adoption is wide, this will cause silent breakage — the old var is silently ignored and the tool appears unauthenticated.

2. Duration validation rejects the defaults it ships
validateDuration only accepts lowercase suffixes (s m h d ms), but buildSearchParams defaults to "1H" and runServicesList defaults to "24H". Those default strings never go through the validator (they're injected after the Nothing path), but a user who writes --since 1H (matching the displayed default) gets a validation error. Either the validator should fold to lowercase before checking, or the defaults should be lowercase "1h" / "24h".

3. validateDurationOrDie error message says --since for --window
runEventsContext calls validateDurationOrDie opts.window but the error produced always reads "--since must match …". Should parameterise the flag name or use a separate validator.

4. aggregateServices looks up "service" but the OTel field is "resource.service.name"

svcIdx = Map.lookup "service" idxMap

If extractColIdxMap preserves the full OTel key (resource.service.name), this lookup returns Nothing for every row and the command always reports zero services. If the server aliases it to "service" in the colIdxMap, fine — but this deserves a comment or a test.

5. docs/cli.md agentic workflow still reads .items
The migration note at the bottom says "pipelines that read .items should switch to .data", but the example in the "Agentic incident workflow" section (step 1) still pipes through .items[]:

monoscope issues list --status open -o json | jq '.items[] | {id,title,service,severity}'

This will break after the normalisation is shipped.

6. Double-parsing in runEventsContext
normalizeEventsResponse val Nothing walks the full logsData array, then withTraceSummary val normalized walks it again. Both start from the raw val. For large payloads this is a noticeable overhead. Consider extracting the index map and rows once and threading them through.

Nits / minor suggestions

• AuthStatus pattern-matches on magic strings "env" and "token" in both the builder and the consumer. A small sum type (two constructors) would make future additions compile-safe.

• FieldTypes.toText via toText . map toLower . drop 2 . show is concise but silently wrong if a constructor is ever renamed with a non-FT-prefixed name. A single comment tying it to the FT prefix convention would help.

• renderParams (debug helper) does not URL-encode values — KQL queries with == and spaces print oddly. Fine for debug-only, but worth a note.

• The toastError type signature carries State.State XWidgetJSON :> es even though neither addErrorToast nor addRespHeaders uses widget JSON in the error path. Worth checking whether that constraint can be dropped; if not, it's fine.

• In withCardFrame, extraAction :: Maybe (Text, Text) — a type alias or small record for (label, url) would make call sites self-documenting.

Positives

• mkPageCtx is a genuine improvement — the repetitive (sess, project) <- sessionAndProject; appCtx <- ask; def{sessM = Just sess, currProject = Just project, config = appCtx.config} was the classic "copy-paste tax".
• deriving (AE.ToJSON) via DAE.CustomJSON '[DAE.FieldLabelModifier '[DAE.CamelToSnake]] is the right call — eliminates drift between Haskell field names and the wire shape.
• deriving (AE.FromJSON, AE.ToJSON, ...) via WrappedEnumSC for FieldTypes / FieldCategoryEnum removes ~40 lines of manual parse/print roundtrip.
• SubPortalResponse / SubResponse using DAE.Rename "dataVal" "data" is exactly the right use of the extension.
• CLI.Validate with doctests is a good pattern for pure helpers.
• CLIE2ESpec.hs with pendingWith graceful degradation when no server is available is clean.

Code Review: Refactor/loc reduction cross cutting

Great PR overall — significant boilerplate reduction, cleaner JSON derivations, better agentic ergonomics. A few issues worth addressing before merge.

Potential bugs

1. aggregateServices / withTraceSummary use hardcoded column keys that may not match server's colIdxMap

-- aggregateServices
svcIdx = Map.lookup "service" idxMap   -- may not exist
tsIdx  = Map.lookup "timestamp" idxMap

-- withTraceSummary
cell "service" r
cell "severity" r

The server's colIdxMap likely uses full OTel paths (resource.service.name, severity.text, etc.). If those keys don't match, svcIdx/tsIdx are Nothing and every row is silently dropped — services list returns an empty table without error. Worth verifying against an actual /api/v1/events colIdxMap response and either using the real keys or documenting why short names work here.

2. Default duration strings bypass the lowercase-only validator

validateDuration whitelists only ["ms", "s", "m", "h", "d"] (lowercase), but buildSearchParams and aggregateServices emit "1H" and "24H" as hardcoded defaults. A user who passes --since 1H gets a validation error; the CLI itself sends the same string. Either normalise the defaults to lowercase or accept uppercase in the validator.

Breaking changes without migration path

3. MONO_* → MONOSCOPE_* env var rename

Existing CI scripts / docker-compose files using MONO_API_KEY, MONO_PROJECT, MONO_API_URL, MONO_AGENT_MODE silently break. A one-release deprecation shim (try MONOSCOPE_* first, fall back to MONO_* with a stderr warning) would soften the cutover. Same applies to MONO_INSTALL_DIR in scripts/install.sh.

4. List response envelope is a breaking change

The docs note {items, total_count} is gone; {data, pagination} is the new shape. No version flag or compat mode. Worth calling this out explicitly in a CHANGELOG / migration note rather than only in docs/cli.md.

Performance concern

5. services list fetches 10,000 events to derive a service list

let params = [("since", fromMaybe "24H" opts.since), ("limit", "10000")]

Now that apiFacets exists and facets resource.service.name is one precomputed HTTP call, runServicesList should use GET /api/v1/facets?field=resource.service.name instead of pulling 10k events and aggregating client-side. The comment acknowledges this is a workaround for a broken /api/v1/metrics endpoint; switching to facets here would fix it properly rather than landing a 10k-row fetch as the permanent implementation.

Fragility

6. FieldTypes.toText via show

getField = toText . map toLower . drop 2 . show

This relies on every constructor being named FT<Something>. If a new constructor is added without the FT prefix, the behaviour is wrong at runtime with no compile-time signal. The WrappedEnumSC machinery already strips the prefix for DB/HI codecs — deriving toText from that same mechanism (or an explicit Map) would be more robust.

Dead bindings

7. Leftover ask @AuthContext after mkPageCtx in some page handlers

dashboardGetH, dashboardTabGetH, dashboardsGetH, and unifiedMonitorOverviewH still bind appCtx <- ask @AuthContext but the bwconf no longer uses appCtx.config (it comes from bw now). GHC -Wunused-binds would catch these — deleting them completes the mkPageCtx cleanup.

Minor nits

• takeFirstEvent does take 1 (toList arr) — V.take 1 arr avoids the list round-trip.
• normalizeList — the Paged guard silently falls through to _ -> v (unnormalised) if the server returns something unexpected. A stderr warning in the fallthrough case would help debugging.
• renderParams in debug output is fine since auth goes via headers, not query params. A short comment so future contributors don't accidentally add sensitive params to query strings.

What's well done

• mkPageCtx is a clear win — eliminates the repeated 4-field BWConfig construction across ~20 handlers.
• deriving (AE.ToJSON) via DAE.CustomJSON for AuthStatusJson, ConfigGetJson, ServiceRow, TraceSummary, and the SubPortalResponse/SubResponse Rename trick — much cleaner than manual instances.
• toastError, withCardFrame, and the hoistEither/maybeToRight/$>/pass micro-cleanups all land well.
• CLI.Validate as a standalone module with doctests is the right structure.
• CLIE2ESpec with per-finding audit assertions is the right test strategy for a CLI audit.

Code Review: Refactor/LOC Reduction Cross-Cutting

Overview

This PR is a large cross-cutting audit and refactor focused on: (1) hardening the CLI for agentic use (stable JSON envelopes, client-side validation, KQL operator fixes, new facets command); (2) eliminating boilerplate in server page handlers via the new mkPageCtx helper; (3) consolidating helpers using existing packages and GHC deriving extensions; and (4) adding an end-to-end test suite for the CLI binary.

Overall this is solid, purposeful work. Most of the reduction is genuine. Comments below focus on correctness concerns and remaining LOC opportunities.

Issues

getField for FieldTypes is fragile

getField = toText . map toLower . drop 2 . show

This relies on constructor names having exactly a 2-character prefix ("FT") and on show producing the constructor name unchanged. If a constructor is renamed, this silently produces wrong strings — the deleted fieldTypeToText was exhaustive and compile-checked. With WrappedEnumSC "FT" FieldTypes already in scope for the codec, reusing that encoding would be safer, or keep the exhaustive match for the HasField instance.

aggregateServices — insertWith argument order is non-obvious

Map.insertWith f new old passes the new value first. The custom bump receives (_, tNew) (n, tOld) where the count lives in the old slot — correct but easy to misread. A clearer alternative avoids the issue entirely:

agg = Map.map (\ts -> (length ts, maximum ts))
      $ Map.fromListWith (<>) [(s, [t]) | (s, t) <- bumps]

Double blank lines in CLI/Commands.hs — several top-level declarations have two blank lines between them (after takeFirstEvent, emitFirstEventId, withTraceSummary, TraceSummary). One blank line is the codebase convention.

filterSchema round-trips through a list unnecessarily

capped = maybe filtered (\n -> Map.fromList (take n (Map.toList filtered))) opts.schemaLimit

Data.Map.Strict exports Map.take :: Int -> Map k v -> Map k v — no list conversion needed:

capped = maybe filtered (`Map.take` filtered) opts.schemaLimit

Minor suggestions

validateAndNormalizeKind — normalizeKind k <$ validateOrDie (validateKind k) is non-obvious (discards the validated Right k, re-applies normalizeKind to the outer k). Slightly clearer:

validateAndNormalizeKind = mapM (fmap normalizeKind . validateOrDie . validateKind)

toastError uses *> instead of >> — both are correct; >> is the conventional operator when the left value is intentionally discarded. Minor style point.

README "Agentic incident workflow" — steps 1 and 5 still use .items[] which the migration note directly above explicitly deprecates in favour of .data[].

Positives

• mkPageCtx is a clear win — removes 5–6 lines of identical boilerplate from ~15 page handlers.
• toastError is a good example of noticing and collapsing a recurrent 2-line pattern.
• deriving (AE.FromJSON, AE.ToJSON) via WrappedEnumSC for FieldTypes/FieldCategoryEnum correctly replaces manual instances.
• SubPortalResponse/SubResponse manual FromJSON instances correctly replaced by DAE.Rename.
• withCardFrame extraction from three widget renderers removes substantial duplication.
• normalizeEventsResponse giving agents named-field objects instead of raw colIdxMap is a meaningful UX improvement.
• hoistEither replacing ExceptT . pure is good use of existing transformers vocabulary.
• E2E test suite is thoughtfully structured — pending-by-default keeps CI green, each audit finding has a named regression test.
• pass replacing pure () in Brick event handlers is idiomatic Relude.

Code Review — Refactor/LOC reduction cross-cutting

Overview

Good, well-motivated PR bundling three real improvements: agentic-CLI hardening (stable JSON envelopes, facets command, cursor pagination, input validation, --debug), server-side mkPageCtx/toastError DRY-up, and deriving-aeson adoption replacing hand-rolled FromJSON/ToJSON. Line-count reduction is genuine. Issues worth addressing before merge:

Bugs

1. CI docs example uses .items after the migration to .data

docs/cli.md CI/Automation section:

monoscope issues list --status open -o json | jq '.items | length'

After normalizeList, items live at .data, not .items. Should be jq '.data | length'. The agentic-workflow section above it correctly uses .data[], so the two sections contradict each other.

2. renderEventsTable in runEventsContext receives raw val, not the enriched value

renderWith mode enriched (renderEventsTable val Nothing)

The JSON/YAML path correctly emits enriched (named fields, --summary traces). The table fallback re-parses raw val, so table output of events context still exposes the raw colIdxMap/logsData indirection rather than the normalised named-field form. Should be renderEventsTable enriched Nothing.

Performance nit

takeFirstEvent round-trips through a list

AE.toJSON (take 1 (toList arr))   -- arr :: V.Vector AE.Value

toList allocates an intermediate [AE.Value]. Prefer AE.Array (V.take 1 arr) — Data.Vector is already imported. Same pattern applies inside emitFirstEventId.

Corrections to the existing review

Three of the five findings in the prior review are not accurate:

• Issue Collab #2 ("validator rejects its own defaults"): validateDurationFor uses T.toLower suffix before the elem check, so "1H" and "24H" pass validation fine.
• Issue Auth0 based cookie authentication #3 (validateDurationOrDie hardcodes "--since"): The signature is validateDurationOrDie flag = mapM_ (validateOrDie . validateDurationFor flag) — the flag name is parameterised. runEventsContext calls validateDurationOrDie "--window" opts.window correctly.
• Issue Support adding and editing and deleting team members from the new project page (and soon, project settings page) #5 (wrong .items[] in agentic workflow): The agentic-workflow section correctly shows .data[]. The broken example is in the CI/Automation section, covered in Bug create project #1 above.
• Issue Project settings page #4 (aggregateServices "service" lookup): The haddock above the function explicitly documents the alias: "The server aliases resource.service.name to the short column name service in the logsData/colIdxMap envelope". No change needed.

Issue #1 (breaking env-var rename MONO_* → MONOSCOPE_*) is valid and still stands.

Minor

• FieldTypes.toText via toText . map toLower . drop 2 . show is concise but silently wrong if a constructor ever loses its FT prefix. A one-line comment tying it to the naming convention would help.
• Several handlers calling mkPageCtx still immediately follow with appCtx <- ask @AuthContext (e.g. dashboardGetH, unifiedMonitorOverviewH). Worth confirming whether appCtx is still needed for anything beyond config, or whether the ask can be dropped.
• renderParams (debug helper) does not URL-encode values — fine for debug use, but worth a note to preempt misuse.

Code Review - Refactor/LOC reduction cross-cutting

Good cross-cutting cleanup. The mkPageCtx extraction alone removes a significant amount of boilerplate, and the deriving-aeson/WrappedEnumSC consolidation is exactly the right approach. A few issues worth addressing:

Bug - doc example contradicts migration note

In docs/cli.md, the CI section was changed to use jq '.items | length' but the migration note immediately below says "Pipelines that read .items should switch to .data". Should be jq '.data | length'.

Performance - unnecessary full Vector->List conversion

In takeFirstEvent (Commands.hs): take 1 (toList arr) materialises the whole Vector before taking the first element. Use V.take 1 arr directly: AE.toJSON (V.take 1 arr). This avoids O(n) allocation for large event arrays.

Stale/orphaned Haddock comment

Around the normalizeEventsResponse/decodeEvents split there are two adjacent -- | doc-comments. The first ("Project the server's positional row...") is not attached to any declaration - it is floating between function definitions. Remove it.

Fragile show-based serialisation in FieldTypes

getField = toText . map toLower . drop 2 . show silently depends on the constructor prefix being exactly 2 chars (FT). Renaming any constructor or changing the prefix breaks the wire format without a compile error. The deleted fieldTypeToText with explicit branches was safer. At minimum add a note, or derive it the same way WrappedEnumSC "FT" does (it already has the strip-prefix logic).

String-literal matching on method in AuthStatus

The non-agent branch matches Just "env" and Just "token" - strings produced two lines above in the same let binding. If the string values ever change the match silently misses. Since envKey and cfg.apiKey are still in scope, prefer matching directly on the original Maybes, or factor into a small ADT.

Minor - buildSearchParams sinceParam

fromMaybe is shorter than the case expression:

sinceParam = fromMaybe (if isJust opts.from || isJust opts.to then "" else "1H") opts.since

Breaking change - env var rename needs callout

MONO_API_KEY -> MONOSCOPE_API_KEY etc. is a breaking change for users who have these set in shell/CI. The docs cover new JSON shapes but there is no explicit "rename your MONO_* vars" notice. Worth a one-liner, or a deprecation period reading both prefixes and warning on the old one.

Nit - validateDuration exported alias

validateDuration (back-compat alias for validateDurationFor "--since") is exported but no call site in this PR requires it over validateDurationOrDie. Consider not exporting it unless it is a public API contract.

Overall the direction is right - mkPageCtx/toastError/normalizeList/deriving-aeson changes are clean wins. The doc bug and the V.take issue should be fixed before merge.

Code Review — Refactor/loc-reduction cross-cutting

Overall this is a solid, well-motivated PR. The KQL fix (== vs :), facets migration, normalised list envelopes, and agent-mode plumbing are all good work. Notes below roughly in priority order.

Breaking changes that need explicit call-outs

1. MONO_* → MONOSCOPE_* env-var rename (Config.hs, Core.hs, docs)

Any existing CI pipeline or dotfile using MONO_API_KEY / MONO_PROJECT / MONO_API_URL / MONO_AGENT_MODE silently breaks — the old name is no longer read, not even as a fallback. The PR description doesn't mention this. At minimum it should be highlighted in the description and the MONOSCOPE_API_KEY section of the docs should call it out as a rename.

2. List JSON shape migration — .items → .data

The migration note in the docs is good, but the PR template checklist item "You are NOT deprecating/removing a feature" is unchecked (and unchecked in the boilerplate, so it may just be the template). Worth confirming intentional deprecation is OK per project policy.

Bugs / correctness

3. isAgentMode fires on any value of CI

isAgentMode = any isJust <$> mapM Env.lookupEnv ["MONOSCOPE_AGENT_MODE", "CLAUDE_CODE", "CI"]

lookupEnv returns Just "" for an unset-but-exported var, and Just "false" for CI=false. Many dev machines have CI set (e.g. direnv, VSCode) which would silently switch to JSON-only mode. Consider isNonEmptyJust:

isAgentMode = any (\v -> isJust v && v /= Just "") <$> mapM Env.lookupEnv [...]
-- or simpler with extras: any (maybe False (not . null)) <$> ...

4. --first / table mode inconsistency

In runEventsSearch:

(False, _) -> renderWith mode sliced (renderEventsTable val validatedOpts.fields)

sliced is truncated to 1 event for JSON, but renderEventsTable val still receives the full val. So --first in table mode shows all events. If --first is JSON-only by design, gate it: when validatedOpts.firstOnly (unless (mode == JSONMode) $ ...), or at least document it.

5. Double Haddock block

In Commands.hs, two -- | doc comments appear back-to-back before DecodedEvents:

-- | Project the server's positional row representation into named-field
-- objects so agents/jq consumers don't need to thread 'colIdxMap'...
-- | Single decode of the events envelope. Sharing the parsed @colIdxMap@...
data DecodedEvents = ...

Only the second one is actually attached to DecodedEvents; the first floats free and will produce a GHC warning. Remove or merge them.

Performance / efficiency

6. decodeEvents called twice in runEventsSearch

normalizeEventsResponse calls decodeEvents internally, and renderEventsTable walks the same val again. runEventsContext already avoids this with an explicit let enriched = case decodeEvents val of ... pattern. Apply the same pattern in runEventsSearch:

withAPIResult cfg "/api/v1/events" params $ \val -> do
  let d = decodeEvents val
      normalized = maybe val (`normalizeDecoded` validatedOpts.fields) d
      sliced     = if validatedOpts.firstOnly || validatedOpts.idOnly then takeFirstEvent normalized else normalized
  case (validatedOpts.idOnly, mode) of
    (True,  _) -> emitFirstEventId sliced
    (False, _) -> renderWith mode sliced (renderEventsTable val validatedOpts.fields)

Style / succinctness (GHC extensions available)

7. parseServiceFacets — use AE.fromJSON instead of manual KM/pattern matching

A small helper type eliminates the hand-rolled txt/int helpers entirely:

data FacetRow = FacetRow {value :: Text, count :: Int}
  deriving stock Generic
  deriving AE.FromJSON via DAE.CustomJSON '[DAE.FieldLabelModifier '[DAE.CamelToSnake]] FacetRow

parseServiceFacets :: AE.Value -> [ServiceRow]
parseServiceFacets v = case AE.fromJSON v of
  AE.Success (obj :: Map Text [FacetRow]) ->
    [ ServiceRow f.value f.count
    | f <- fold (Map.lookup "resource.service.name" obj) ]
  _ -> []

Shorter, type-safe, and consistent with how MetricsData is decoded.

8. withMetricsData silently swallows the parse error

AE.Error _ -> onFail

The _ discards a potentially useful message. Even a printDebug here would help when debugging unexpected server shape changes:

AE.Error msg -> printDebug ("metrics decode failed: " <> toText msg) >> onFail

But this requires threading Environment through withMetricsData. Alternatively just leave a TODO comment.

9. fmap vs <$> consistency in Main.hs

-- new code:
idsOpt = T.splitOn "," <$> strOption (...)
-- vs existing in int64IdsOpt (same PR):
mapMaybe (readMaybe . toString) . T.splitOn "," <$> strOption (...)

Both are fine; just noting consistency is already there.

10. Double blank lines

Several spots introduce two blank lines between top-level definitions (after parseServiceFacets, emitFirstEventId, takeFirstEvent, withTraceSummary). HLS/fourmolu typically uses one. Minor, but worth a pass before merge.

Positive highlights

• hoistEither / maybeToRight / fromRight / $> / pass replacements are all idiomatic — good use of Relude.
• DecodedEvents sharing pattern in runEventsContext is the right call for large payloads.
• normalizeListE returning Either with both arms carrying a value is a bit unusual (it's not an error channel), but it's well-documented and normalizeList = either id id is clean.
• validateDurationOrDie using mapM_ to skip on Nothing is concise and correct.
• HTTP body forwarding in httpExToError for server validation errors is a nice UX win.
• Shell completion error handling is a clear improvement.
• README and docs updates are thorough.

Code Review — PR #365: Refactor/LOC reduction cross-cutting

Overall: Solid, well-motivated refactor. The direction is right — less boilerplate, stable agent-facing JSON shapes, and cleaner error paths. A few specific issues worth addressing before merge.

Positives

• mkPageCtx cleanly eliminates 8+ copies of the (def :: BWConfig){sessM = …, currProject = …, config = …} pattern across page handlers. Good use of a shared helper.
• deriving-aeson CamelToSnake / Rename for AuthStatusJson, ConfigGetJson, TraceSummary, SubPortalResponse, SubResponse correctly replaces hand-rolled FromJSON/ToJSON instances. This is exactly the right tool.
• WrappedEnumSC via-deriving for FieldTypes/FieldCategoryEnum drops fieldTypeToText, parseFieldTypes, parseFieldCategoryEnum, and the manual JSON instances — good consolidation.
• validateEventsOpts / CLI.Validate — putting all input validation in one place with clear exit messages is the right pattern for agent-friendly CLIs.
• withMetricsData — decoding to the typed MetricsData rather than AE.Value makes renderMetricsTable/renderSparkline/checkAssertion simpler and safer.
• normalizeListE returning Either so callers can log unknown envelope shapes is thoughtful.
• toastError helper removing the repetitive addErrorToast x Nothing >> addRespHeaders y pair is a clear win.
• Replacing pure () with pass (from Relude) throughout UI handlers is idiomatic.

Issues

1. getField = toText . map toLower . drop 2 . show is fragile

-- src/Models/Apis/Fields.hs
instance HasField "toText" FieldTypes Text where
  getField = toText . map toLower . drop 2 . show

This relies on show producing exactly "FTString", "FTNumber", etc. and the prefix always being exactly 2 chars. GHC doesn't guarantee show for a derived Stock instance follows a naming convention — it's convention, not a contract. More critically, it will silently produce wrong output if a constructor is renamed or a new prefix added. The deleted fieldTypeToText was explicit and total. Consider at least adding a test, or using the existing WrappedEnumSC "FT" FieldTypes mechanism (it presumably already handles this mapping for FromField/ToField/JSON) rather than reimplementing via show.

2. parseServiceFacets inline parsers vs aeson

toRow (AE.Object o) = ServiceRow <$> txt o "value" <*> int o "count"
txt o k = case KM.lookup k o of Just (AE.String s) -> Just s; _ -> Nothing
int o k = case KM.lookup k o of Just (AE.Number n) -> Just (round n); _ -> Nothing

The server's FacetValue type already has a ToJSON instance. Defining ServiceRow as a rename of FacetValue (or just deriving FromJSON for ServiceRow directly with CamelToSnake) and using AE.fromJSON would replace all three helpers with a one-liner and be compile-checked against the server type.

3. normalizeDecoded — silent empty string on missing fields

fromMaybe "" (Map.lookup k d.idxMap >>= \i -> listToMaybe (drop i r))

If idxMap has a key but the row is shorter than i, listToMaybe (drop i r) returns Nothing and the field silently becomes "". This masks sparse/malformed rows in agents' output with no warning. Consider Nothing/null instead of "" so agents can distinguish "field present and empty" from "field missing".

4. --debug sets a process-wide env var

-- cli/Main.hs
when global.debugFlag $ setEnv "MONOSCOPE_DEBUG" "1"

setEnv is process-global and not thread-safe. This is probably fine since it's set once at startup before any threads are spawned, but it's worth a comment explaining why (or alternatively, thread a Bool through the config).

5. renderParams leaks query values to stderr unredacted

The comment says "if you ever add a credential to a query param, redact it here" — but there's no enforcement. MONOSCOPE_API_KEY is currently sent as a header (safe), but a future dev could add it as a param and the debug output would leak it. The comment is a start; a stronger guard would be an allowlist of safe param names or a type-level distinction.

6. emitCompletion liftIO wrapping is asymmetric

emitCompletion shell = case shell of
  "bash" -> emit "--bash-completion-script"
  ...
  other -> liftIO $ do  -- only the error branch is in liftIO directly
    ...
  where
    emit shellArg = liftIO $ ...

This is fine but slightly inconsistent — both branches end up in liftIO, so the function could be :: IOE :> es => or just :: IO () hoisted once at the call site. Minor.

7. Missing helpCases entry for FacetsCmd in CLIBinarySpec

The PR adds command "facets" … in Main.hs and the docs mention CLIBinarySpec.helpCases, but the diff doesn't show CLIBinarySpec.hs being updated. If the smoke-test list isn't extended, the new command won't be covered by the "exit 0 for every subcommand --help" regression guard described in the docs.

8. dashboardGetH / dashboardTabGetH still call ask @AuthContext after mkPageCtx

-- src/Pages/Dashboards.hs
dashboardGetH ... = do
  (_, project, bw) <- mkPageCtx pid
  appCtx <- ask @AuthContext   -- ← still here

mkPageCtx already reads AuthContext internally and returns bw.config. These call sites then re-read AuthContext for the same config. The residual ask @AuthContext should be removed and bw.config used directly (as other migrated handlers already do).

Minor / Style

• Double blank lines before several top-level definitions (parseServiceFacets, takeFirstEvent, emitFirstEventId, withTraceSummary, TraceSummary) — one blank line is the Haskell convention.
• extractNumericRows removed from Pkg.CLIFormat import but the function itself is apparently unused now; verify it's also removed from the export list / module (not visible in this diff).
• The ValidateDuration doctest examples reference validateDurationFor in comments but the exported name is validateDuration / validateDurationOrDie — make sure the doctest headers match the actual exported names so they run.
• intercalate "\n" in eventsSearchExamples / facetsExamples could use unlines (drops the trailing-newline difference but is idiomatic).

Code Review — PR #365: Refactor/loc reduction cross cutting

Good overall direction. This PR fixes real bugs (KQL operators, broken --service/--level filters, events get wrong query), adds solid agent-mode ergonomics, and meaningfully reduces boilerplate (mkPageCtx, toastError, WrappedEnumSC derivations). Some specific issues below.

Potential Bug — withTraceSummary service column name

In Commands.hs, withTraceSummary calls cell "service" r to extract service names from raw event rows. The idxMap is built from the server's colIdxMap, where column names match the telemetry field paths (e.g. resource.service.name, not service). This means cell "service" almost always returns Nothing, so all trace summaries will have services: [] regardless of actual traffic.

-- likely wrong:
cell "service" r
-- should probably be, with fallback:
cell "resource.service.name" r <|> cell "service" r

Breaking Change — MONO_* → MONOSCOPE_* env vars

The rename is correct long-term, but it silently breaks all existing users who export MONO_API_KEY, MONO_PROJECT, etc. No deprecation shim, no warning on first use of the old name. Consider at minimum:

• resolveConfig could check both names (old as fallback) and emit a deprecation warning to stderr
• Or document this prominently in the PR as a required migration step, with the exact old→new mapping

isAgentMode truthy check — minor simplification

-- current (three exact strings for case variants):
truthy = maybe False (\v -> not (null v) && v `notElem` ["false", "False", "FALSE", "0"])

-- simpler (one case-insensitive check):
truthy = maybe False (\v -> not (null v) && map toLower v `notElem` ["false", "0"])

filterSchema — prefer Map.take

-- current (list round-trip):
Map.fromList (take n (Map.toList filtered))

-- cleaner (containers >= 0.6.3, already in scope):
Map.take n filtered

validateDurationFor not wired up to doctests yet

The module comment in CLI/Validate.hs says the doctests "run as part of the lib's doctest suite once exposed; until then they document expected behavior in-source." If they're not actually executing in CI, they provide false confidence. Worth either wiring them up now or replacing with HSpec unit tests in CLIBinarySpec.

runEventsGet — no validation of event ID

The --id-only path and runEventsGet construct KQL strings via direct concatenation ("id==\"" <> opts.eventId <> "\"") without validating the ID format. A malformed ID (e.g. containing ") would produce an invalid KQL query and an opaque server 400. The new validateUuid in CLI.Validate already exists — wire it into runEventsGet / EventsGetOpts parsing for free client-side error messages.

Small wins leveraging existing packages / extensions

• AuthStatus method field: using Text values "env"/"token" for a two-variant field means the pattern match is String-based, not exhaustive. An ADT (even a local data AuthMethod = EnvKey | StoredToken) would close the open case. The JSON encoding stays Text via a custom ToJSON, still one line with deriving-aeson.

• withTraceSummary merge: the merge tuple accumulation is dense. Data.Map.unionWith or Data.Map.insertWith on a named record would be more readable at the cost of ~2 lines.

• Dashboards.hs still calls ask @AuthContext after mkPageCtx in dashboardGetH and dashboardTabGetH (appCtx <- ask @AuthContext on the line after (_, project, bw) <- mkPageCtx pid). Since mkPageCtx already reads AuthContext internally, these callers only need appCtx.env for the time-picker. Consider exposing EnvConfig from BWConfig (it's already in bw.config) rather than doing a second ask.

Nits

• The --debug flag implementation (setEnv "MONOSCOPE_DEBUG" "1" in main) leaks into child processes. That's probably fine for a debug flag, but worth noting.
• renderParams comment mentions "if you ever add a credential to a query param, redact it here" — but there's no enforcement. A TODO or a compile-time marker would make this easier to catch later.
• Several handlers in Dashboards.hs / LogExplorer/Log.hs still have a trailing appCtx <- ask @AuthContext after adopting mkPageCtx. These are now dead bindings if appCtx is only used for appCtx.config (already in bw.config). Clean them up while the pattern is fresh.

Summary: the core refactor is sound and the bug fixes are valuable. The cell "service" issue in withTraceSummary is the one change I'd want verified before merge. The env-var rename needs explicit migration guidance. Everything else is polish.

Code Review – Refactor/loc reduction cross cutting

Overview

Good LOC-reduction pass across the CLI and server-side page handlers. The dominant gains are mkPageCtx (eliminates ~20 copy-pasted (sess,project)/appCtx/def{...} blocks), WrappedEnumSC deriving for FieldTypes/FieldCategoryEnum (~55 lines gone), deriving-aeson for stable JSON shapes, and the toastError combinator. The agentic pipeline additions (facets, --first/--id-only, --summary, --debug, stable envelope normalization) are well-motivated.

Breaking change – env var rename

MONO_* → MONOSCOPE_* is a silent breaking change for every existing user with those vars in dotfiles/CI. The PR description doesn't mention it and the docs just swap the names without a migration note. Worth at minimum a deprecation warning (check MONO_API_KEY and emit a warning if set but MONOSCOPE_API_KEY isn't) or a changelog entry.

reqOpts indentation is misleading

reqOpts cfg params =
  W.defaults
    & W.header "Accept"
    .~ ["application/json"]
      & addAuth cfg.apiKey
      & addProjectId cfg.projectId
      & addParams params

& addAuth is indented deeper than & W.header "Accept", implying it's part of the RHS of .~ rather than a continuation of the & chain. Fourmolu should have caught this – the original one-liner was clearer. Either put .~ on the same line as & W.header "Accept" or keep it on its own line at the same depth as the surrounding & applications.

Duplicate positional-lookup logic

withTraceSummary defines:

cell :: Text -> [Text] -> Maybe Text
cell field r = Map.lookup field d.idxMap >>= \i -> listToMaybe (drop i r)

normalizeDecoded defines:

cellAt r k = Map.lookup k d.idxMap >>= \i -> listToMaybe (drop i r)

These are identical modulo argument order. Extract to DecodedEvents:

cellAt :: DecodedEvents -> Text -> [Text] -> Maybe Text
cellAt d field row = Map.lookup field d.idxMap >>= \i -> listToMaybe (drop i row)

and reuse in both callers.

filterSchema – prefer Map.take

capped = maybe filtered (\n -> Map.fromList (take n (Map.toList filtered))) opts.schemaLimit

Map.toList + take + Map.fromList is O(n log n). Map.take is O(log n + k):

capped = maybe filtered (`Map.take` filtered) opts.schemaLimit

Already imported Data.Map.Strict.

Minor: parseServiceFacets – fold on Maybe is non-obvious

[ServiceRow f.value f.count | f <- fold (Map.lookup "resource.service.name" m)]

fold on Maybe [a] is valid (Foldable Maybe) but surprising to readers. fromMaybe [] reads more clearly:

[ServiceRow f.value f.count | f <- fromMaybe [] (Map.lookup "resource.service.name" m)]

Minor: toastError with "" as response

Nothing -> toastError "Member not found" ""

The empty string is an RespHeaders "" – presumably a no-op body. A type alias or noContent would make the intent clearer at the call site, but this is cosmetic.

Minor: capFacets – third clause is dead for vectors

capFacets (Just n) (AE.Object obj) =
  AE.Object $ flip KM.map obj $ \case
    AE.Array xs -> AE.Array (V.take n xs)
    other -> other
capFacets _ v = v

The other -> other passthrough inside the object map is fine for non-array fields. The outer capFacets _ v = v also handles Nothing, but the Nothing case is already handled by the first clause guard on Just n. Adding a comment or merging the Nothing case explicitly makes exhaustiveness clearer. Minor.

Tests

CLI.CLIE2ESpec is added (good) but gated behind live-server env vars – this is the right approach for a CLI that talks to an HTTP API. The binary smoke tests in CLIBinarySpec catch parser wiring errors for free. Would be worth adding FacetsCmd to helpCases in CLIBinarySpec since a new top-level command was added.

Summary

The positive changes here far outweigh the issues: mkPageCtx alone removes hundreds of lines of boilerplate, the WrappedEnumSC pattern is correctly extended, the agent-mode pipeline is well-thought-out, and error surfacing from HTTP bodies is a meaningful DX improvement. The items above are all fixable in a follow-up or inline – the breaking env-var rename is the only one I'd want addressed before merge.

Code Review PR 365 - see full review below

Code Review PR 365 - Bugs/Correctness

1. HasField 'toText' FieldTypes uses show for serialization (fragile)

In src/Models/Apis/Fields.hs: getField = toText . map toLower . drop 2 . show

Works for all current constructors, but this is an implicit contract on Show. If a constructor is added with <=2-char suffix or renamed without the FT prefix, it silently produces wrong output at runtime. The original explicit fieldTypeToText was safer. At minimum add a doctest asserting the output; better: delegate to WrappedEnumSC which already owns the wire encoding.

2. parseDurationMs only handles lowercase suffixes but validateDurationFor accepts uppercase

validateDurationFor accepts '1H', '24H' (case-insensitive), but parseDurationMs only matches lowercase suffixes. parseDurationMs '5M' falls through to readMaybe and returns 5000ms instead of 300000ms. Only affects --watch on metrics chart, but validateDurationOrDie will silently accept '5M' and pass it to parseDurationMs which misreads it. Fix: T.toLower the input at the start of parseDurationMs.

3. reqOpts formatting in CLI/Core.hs obscures operator precedence

The .~ is split across three lines making it look like a prefix to the next & chain. Formatter artifact worth overriding.

4. Incomplete mkPageCtx migration in Dashboards.hs

In dashboardGetH/dashboardTabGetH: appCtx <- ask @authcontext is still present alongside (_, project, bw) <- mkPageCtx pid, with bw.config.hostUrl used in one place and appCtx.env.apiKeyEncryptionSecretKey in another. ask is still needed for the encryption key, but the asymmetry is surprising; a comment explaining why would help.

Code Review — PR #365 (Refactor/LOC reduction cross-cutting)

Good overall direction. The agentic-pipeline additions are well-motivated and the agent-mode detection fix (CI=false is now treated as falsy) is a genuine correctness improvement. A few specific notes:

Improvements worth making before merge

1. withTraceSummary — cell helper is O(n) per lookup, called in a hot loop

cell field r = Map.lookup field d.idxMap >>= \i -> listToMaybe (drop i r)

drop i r on a [Text] row is O(i) per call, and cell is called three times per row (trace_id, service, errors). For large event windows this adds up. Either keep idxMap resolved to indices once before the fold, or use V.fromList r and index directly. The extractRows path already has this shape.

2. validateDurationFor does not handle the "1H" uppercase case correctly

T.toLower suffix `notElem` ["ms", "s", "m", "h", "d"]

This is fine — T.toLower "H" == "h" — but the doctest says validateDurationFor "--since" "1H" returns Right "1H", which is correct. The issue is that the services list hardcodes "24H" (uppercase) as a default, but validateDurationOrDie would reject user-supplied "1H" as... wait, it wouldn't, T.toLower handles it. Actually this is fine. Ignore this point.

3. normalizeListE — the Paged branch silently drops fields not in the target shape

If the server adds a field to its Paged envelope, the normaliser silently discards it. This is probably intentional (stable output contract), but the comment in runList says "server returned an envelope shape we don't recognise" for the Left case. The real risk is the opposite: the server does return a Paged but AE.fromJSON @(Paged AE.Value) fails because a required field changed — that falls through to Left v with the confusing debug message "did not match {data, pagination} or Paged envelope". Consider logging the decode error instead of the generic message when AE.Error msg is produced.

4. httpExToError — body is ByteString (strict) but the variable is already named body in the pattern

httpExToError (HttpExceptionRequest req (StatusCodeException resp body)) =

StatusCodeException's second field is the truncated body (at most 1024 bytes from http-client). T.strip (decodeUtf8 body) will mis-decode binary/gzipped error bodies. Wrap with decodeUtf8With lenientDecode (from Data.Text.Encoding) rather than the strict decodeUtf8 to avoid a runtime exception on non-UTF-8 bodies.

5. emitFirstEventId — exits with a non-zero message but no exit code is set explicitly

_ -> printError "no events matched" >> liftIO exitFailure

This is fine — exitFailure does set exit code 1. But the --id-only path in runEventsSearch routes through emitFirstEventId sliced, where sliced is the normalised envelope. If decodeEvents returns Nothing (shape mismatch), sliced == val (original value), and the AE.Object branch will still fail gracefully. OK, no bug here.

6. isAgentMode — map toLower is from Data.Char but Relude re-exports Data.Text.map

truthy = maybe False (\v -> not (null v) && map toLower v `notElem` ["false", "0"])

v is String (from Env.lookupEnv), so Data.Char.toLower is correct here. But null is ambiguous — Relude exports Data.Text.null and this is String. With NoImplicitPrelude + OverloadedStrings, GHC should pick Prelude.null :: [a] -> Bool since v is String, but it's worth making this explicit: not (null (v :: String)) or just v /= "" to be unambiguous.

Minor / style

• The blank lines between every top-level binding are inconsistent with the existing file style and add noise to the diff. The project's convention (seen in the pre-PR code) is one blank line between bindings, not two.
• data AuthStatusJson and data ConfigGetJson are single-use structs whose only purpose is a specific ToJSON instance. These could be inlined with AE.object [...] at the call site (saves ~10 lines each and removes the Generic boilerplate). The CamelToSnake path is nicer when the struct is reused or grows fields, but for a two-field auth status response it's over-engineering.
• runConfigGet now takes an explicit OutputMode argument, but runConfigSet/runConfigInit don't — the asymmetry is a bit odd. Not a blocker but worth noting for a follow-up.
• normalizeList = either id id . normalizeListE — either id id is fromRight (fromLeft undefined) — just use fromEither from Relude if available, or keep either id id which is clear.
• In CLI/UI.hs: currentValue = T.strip . unlines . BE.getEditContents — unlines is Prelude.unlines :: [String] -> String, not Data.Text.unlines. Since BE.getEditContents :: Editor t n -> [t] and t ~ Text, this will produce a type error unless OverloadedStrings resolves unlines to a Text variant. Verify this compiles — or use T.unlines explicitly to be safe.

Security / correctness

• KQL injection via foldFiltersIntoQuery: the new q v = "\"" <> T.replace "\"" "\\\"" v <> "\"" escapes double-quotes but not backslashes within the value. If a service name contains a literal backslash (unusual but possible), q "foo\\bar" produces "foo\\bar" in the KQL string, which may or may not be valid depending on the server parser. Mirror the runEventsGet fix (T.replace "\\" "\\\\" first) for consistency.

What's good

• The hoistEither refactor in runAuth is exactly the right use of the errors package.
• parseServiceFacets decoding through Fields.FacetValue so wire changes are caught at compile time is a nice correctness win.
• validateDurationOrDie / validateAndNormalizeKind centralising validation before the poll loop in runEventsTail is the right place to catch errors.
• Renaming MONO_* → MONOSCOPE_* throughout is a breaking change but the right long-term call; the docs update is consistent.
• pass replacing pure () is idiomatic Relude.

Code Review: Refactor/LOC Reduction Cross-Cutting

Good PR overall — clear intent, consistent direction, real bug fixes bundled in. Notes below, prioritised by impact.

Bugs / Correctness

FieldTypes.toText via show is fragile.

-- Fields.hs
getField = toText . map toLower . drop 2 . show

This relies on show producing "FTString", "FTNumber", etc. — a GHC implementation detail. If a constructor is renamed or a Show instance is derived differently (e.g. via showsPrec with a wrapper), drop 2 silently produces wrong output. The removed fieldTypeToText was correct-by-construction; the WrappedEnumSC "FT" FieldTypes derivation already handles the prefix so the ToJSON/FromJSON round-trip is fine — it's only this HasField "toText" instance that is now fragile. At minimum add a {-# ANN ... -#} guard or a HUnit test that FTString.toText == "string".

decodeEvents / normalizeDecoded always rewrites the cursor field as Null on missing. fromMaybe AE.Null (KM.lookup "cursor" d.raw) means a server response with no cursor key emits "cursor": null — fine — but so does a server that sends "cursor": "2026-…" if the key name changed. Double-check that the server actually uses "cursor" (not "next_cursor" or similar); the raw passthrough in withAPIResult would show the mismatch under --debug, but the normalised path silently drops it.

withTraceSummary double-falls-through on non-Object input.

withTraceSummary _ v = v  -- catchall

This is correct, but the opts.summary flag is only checked in runEventsContext. If withTraceSummary is ever called from another site without that guard, it silently skips. Minor, but worth a note.

Code Quality / Succinctness

normalizeListE can use asum / <|> instead of nested case. The three-branch case on AE.Value is already clean, but the Paged-decode branch shadows a successful {data, pagination} check because the first guard matches AE.Object unconditionally. The second branch also matches AE.Object:

AE.Object _
  | AE.Success (p :: Paged AE.Value) <- AE.fromJSON v -> ...

This means any AE.Object that already has data+pagination is tested against the Paged decoder too (it just falls through). Harmless but worth a comment, or restructure with otherwise in the first guard.

withMetricsData could be bitraverse-style. The pattern

case AE.fromJSON val of
  AE.Success md -> onOk md
  AE.Error msg  -> printDebug ... >> onFail

is fine, but this is exactly what AE.fromJSON + either/withRight idiom handles. Since AE.Result is a Monad, onOk <$> AE.fromJSON val and either … onOk (AE.toEither result) would both be shorter. Not a blocking issue.

emitFirstEventId uses a multi-layer pattern guard where preview would be cleaner:

-- current
AE.Object obj
  | Just (AE.Array arr) <- KM.lookup "events" obj
  , Just (AE.Object e)  <- arr V.!? 0
  , Just (AE.String i)  <- KM.lookup "id" e -> putTextLn i

-- alternative (with lens already in scope)
val ^? key "events" . nth 0 . key "id" . _String

Data.Aeson.Lens is already a transitive dep via BackgroundJobs.hs. Using it here makes the intent obvious and eliminates the guard chain.

isAgentMode's truthy helper re-implements Data.Char.toLower on a String — T.toLower on Text would be more consistent with the rest of the file (though Data.Char.toLower on String is correct).

validateDurationFor strips in the middle of validation but validates the raw input first:

| T.null t = Left ...       -- checks raw t
| otherwise = let trimmed = T.strip t  -- strips after the null check

T.null " " is False so a whitespace-only string passes the null check, then T.null trimmed is checked later only implicitly (digits would be empty). This works correctly due to the T.null digits check but it's a subtle ordering. A pre-strip at the top would make the intent clearer.

Performance

parseServiceFacets uses fold on a Maybe [FacetValue] — that's fine for small lists but it folds the entire result from Map.lookup into a single list with fold (Map.lookup …). The intent is fromMaybe []; using fromMaybe [] (Map.lookup …) is O(1) on the Maybe allocation and makes the intent explicit.

withTraceSummary builds Map.fromListWith merge over all rows every call. For events context the default window is 5m so row counts are small — this is fine. No concern.

Breaking Change / Migration Risk

MONO_* → MONOSCOPE_* env-var rename is a breaking change for anyone using the old names in CI or scripts. The docs include a migration note in the table but there's no compatibility shim (fallback read of the old names). Worth a one-cycle deprecation (lookupEnv "MONO_API_KEY" >>= maybe (lookupEnv "MONOSCOPE_API_KEY") (pure . Just)) or at minimum a prominent callout in the PR description/changelog.

config get now requires an OutputMode argument — the call site in Main.hs passes it correctly, but the signature change means any direct caller of runConfigGet outside Main.hs must be updated. Fine for a mono-repo but worth a grep to confirm there are no other callers.

Naming / Style

• ConfigGetOpts changed from data to newtype — correct, good.
• pure () → pass throughout — good use of Relude.
• \_ -> pure () → const action in withSpinner — cleaner.
• Module export lists reformatted to trailing-comma style — consistent with the rest of the codebase.
• ExceptT . pure → hoistEither — good.
• maybe (Left …) Right → maybeToRight — good use of existing combinators.
• either (const emptyConfig) id → fromRight emptyConfig — good.
• when … notElem → unless … elem in Anomalies.hs — good.
• NE.head → head in deduped — fine since NE.groupBy returns non-empty groups.

Test Coverage

The docs describe three test layers (doctests, binary smoke tests, e2e). The new CLIE2ESpec.hs is referenced in monoscope.cabal but not in the diff — was it already in the tree? If it's new, it should be in the diff. If it's existing, a pointer to the file would help reviewers know which audit findings are actually covered by a regression test.

validateDurationFor has doctests inline but they aren't executed unless the module is exposed via the doctest suite. Consider adding the module to the doctest runner, or promote the examples to CLISpec.hs unit tests.

Minor

• runConfigGet comment references B9 — these numbered codes are useful for the author but meaningless to future readers. Drop or explain inline.
• renderParams note about not URL-encoding: correct and useful, but the comment says "if you ever add a credential to a query param, redact it here" — this is a latent footgun. Consider redacting any param whose key contains "key" or "token" proactively.
• Extra blank lines between top-level declarations throughout — consistent within this PR but differs from the existing style in some files. Not blocking.

Code Review — Refactor/LOC Reduction Cross-Cutting

Good overall direction. The PR tightens the agentic surface (stable JSON envelopes, client-side validation, --first/--id-only, facets, --summary), renames env vars consistently, and reduces boilerplate through deriving-aeson, pass, and better use of existing combinators.

Bugs / Correctness

reqOpts formatting broke the operator chain (CLI/Core.hs):

-- current (wrong — & associates left, so .~ sees the wrong LHS)
W.defaults
  & W.header "Accept"
  .~ ["application/json"]
    & addAuth cfg.apiKey

The & / .~ are misaligned — .~ now applies to the result of W.header, not to W.defaults. This silently drops the Accept header on every request. Should be:

W.defaults
  & W.header "Accept" .~ ["application/json"]
  & addAuth cfg.apiKey
  & addProjectId cfg.projectId
  & addParams params

withTraceSummary uses Map.fromListWith merge with Set.union (<> on Set)—this is correct but merge (s1,c1,e1) (s2,c2,e2) uses <> where the first arg is the new value and second is the accumulated value in fromListWith, which is fine for Set but easy to get wrong if someone adds a field later. A type alias or comment would help.

normalizeDecoded drops fields not in idxMap when --fields is given — specifically, the filter filter (\Map.member` d.idxMap)silently ignores unknown field names. An agent passing--fields id,trace_idwhentrace_idis absent from the column set will get{"id": ...}and no warning. AprintDebug` here would help self-correction.

Succinctness / Use Existing Packages

validateDurationFor re-implements T.stripPrefix-style digit-span. The text package's T.span isDigit is already used — that's fine — but the notElem list could be a Set for O(1) lookup (minor):

T.toLower suffix `notElem` ["ms","s","m","h","d"]
-- →
T.toLower suffix `Set.notMember` Set.fromList ["ms","s","m","h","d"]

Or define a top-level validSuffixes constant.

withMetricsData is just maybe/traverse over AE.fromJSON — consider:

withMetricsData val onFail onOk = case AE.fromJSON val of
  AE.Success md -> onOk md
  AE.Error msg  -> printDebug ("metrics decode failed: " <> toText msg) >> onFail

This is already what the PR does, so fine; but the helper could simply be:

withMetricsData :: … => AE.Value -> Eff es () -> (MetricsData -> Eff es ()) -> Eff es ()
withMetricsData = either (const …) … . AE.fromJSON  -- not meaningful without the debug line

Keep as-is.

emitFirstEventId pattern-matches deeply on AE.Value. With lens-aeson (already a transitive dep via wreq):

emitFirstEventId v = maybe notFound putTextLn (v ^? key "events" . nth 0 . key "id" . _String)
  where notFound = printError "no events matched" >> liftIO exitFailure

takeFirstEvent / takeFirstRow are structurally identical modulo the key name — could unify:

takeFirstIn :: AK.Key -> AE.Value -> AE.Value
takeFirstIn k (AE.Object obj) = AE.Object $ KM.adjust (\case AE.Array a -> AE.Array (V.take 1 a); x -> x) k obj
takeFirstIn _ v = v

normalizeListE is clear but the Object branch duplicates pagination key names. A helper paginationObj would reduce repetition between the Paged-success arm and the bare-Array arm.

parseServiceFacets uses fold (Map.lookup …) — idiomatic but foldMap id / fromMaybe [] are both in scope. fromMaybe [] (Map.lookup …) is slightly more readable.

Design / API Surface

Breaking env var rename without a compatibility shim: MONO_API_KEY → MONOSCOPE_API_KEY, etc. Existing CI pipelines will break silently (no warning, just "not authenticated"). Even a one-release compatibility alias (envApiKey <- Env.lookupEnv "MONOSCOPE_API_KEY" <|> Env.lookupEnv "MONO_API_KEY") would reduce friction. The PR description should call this out explicitly as a breaking change.

runConfigGet now takes OutputMode as an extra argument — this changes the public function signature exported from CLI.Commands. If anything outside Main.hs calls it directly, it will break at the call site. Check for external callers (skill scripts, tests).

validateDurationFor "" returns Left "… must not be empty" — the "" case is triggered when a user passes --since "". That's valid (no lookback specified); consider treating empty the same as Nothing rather than erroring.

isAgentMode checks CI — this will enable agent mode (JSON-only, no device-code login) inside any standard CI, even for human-invoked commands. The truthy/falsy normalization helps, but CI=false won't save you in GitHub Actions where CI is always "true". This is pre-existing behaviour, but widening to MONOSCOPE_AGENT_MODE as the preferred signal is correct.

--debug uses setEnv "MONOSCOPE_DEBUG" "1" at process start — the comment notes it's not thread-safe but safe here. Worth confirming the runHTTPWreq/effectful runtime doesn't spawn threads before run is called.

Test Coverage

• CLI/Validate.hs doctests are in-source but not wired into the test suite yet (per the doc note "once exposed"). They should be added to CLIBinarySpec or a dedicated doctest target so they actually run in CI.
• normalizeListE and withTraceSummary are non-trivial and fully pure — unit tests would be cheap and give confidence for the stable-envelope contract.
• The foldFiltersIntoQuery doctests are updated but the --service "my app" case now produces resource.service.name=="my app" (double-quoting with q helper). The doctest in the diff still shows the new expected output — good. Verify the doctest suite actually exercises it.

Minor

• Extra blank lines between every definition inflate line count; a single blank between top-level definitions is idiomatic Haskell.
• Data.Char (toLower) is in scope via Relude as toLower from Data.Char — the explicit import is fine but redundant with Relude's re-export if it re-exports it.
• Module export lists: switching from the multi-line ( foo , bar ) where style to ( foo, bar, ) where with trailing comma is consistent with GHC2021 trailing-comma style — fine, just make sure the formatter is configured to not fight it.

Summary: The direction is right and the agent-mode story is significantly better. Fix the reqOpts operator-chain bug before merging (it breaks auth headers), add a migration note for the env var rename, and consider wiring up the doctests. The LOC reduction is real and the use of deriving-aeson, pass, hoistEither, <<$>>, and maybeToRight is idiomatic — good leveraging of the existing package set.

Code Review

Overview

Solid LOC-reduction pass. The three new abstractions (mkPageCtx, toastError, withCardFrame) are the heart of the PR and they pull their weight well. The removal of hand-rolled enum serializers in favour of deriving via WrappedEnumSC and the switch to typed MetricsData decoding are both good examples of using what the codebase already has.

Good

• mkPageCtx eliminates the 4-line (sess, project) <- …; appCtx <- ask; let bwconf = def{…} block at 20+ handler sites — unambiguously the right call.
• toastError removes the recurring addErrorToast msg Nothing >> addRespHeaders payload pairing at 17 call sites.
• withCardFrame cleanly unifies three nearly-identical widget card shells.
• FieldTypes / FieldCategoryEnum dropping ~30 lines of manual FromJSON/ToJSON/FromField in favour of deriving (AE.FromJSON, AE.ToJSON, …) via WrappedEnumSC is exactly the kind of change this PR is supposed to make — good.
• renderMetricsTable / renderSparkline now accept a typed MetricsData instead of extracting from AE.Value manually; withMetricsData wraps the decode-or-fallback cleanly.
• New CLIE2ESpec with opt-in env-var gating is a good pattern for integration tests that need a live server.

Issues

1. Incomplete toastError migration — src/Pages/Anomalies.hs:831,833,853,855

Four call sites were missed:

-- still unconsolidated in Anomalies.hs
Nothing -> addErrorToast "Error not found" Nothing >> addRespHeaders mempty
| err.projectId /= pid -> addErrorToast "Error not found for this project" Nothing >> addRespHeaders mempty

These should become toastError "Error not found" mempty etc. Minor, but inconsistent given the stated goal.

2. NE.head → head (src/Pages/Anomalies.hs:2878) — safe but loses intent

-- before
deduped = map NE.head $ NE.groupBy ((==) `on` dedupKey) $ sortOn dedupKey raw
-- after
deduped = map head $ NE.groupBy ((==) `on` dedupKey) $ sortOn dedupKey raw

NE.groupBy returns [NonEmpty a], and relude's head :: NonEmpty a -> a is total, so this is technically safe. But NE.head makes the invariant self-documenting at the call site. Worth keeping NE.head here.

3. apiFacets — magic number & loose return type (src/Web/ApiHandlers.hs)

defaultFrom = fromMaybe (addUTCTime (-86400) now) fromT

The -86400 literal (24 h in seconds) should be a named constant — it also appears in other handlers. Consider defaultLookback = nominalDay (from Data.Time).

The function returns AE.Value where it could return FacetData (already defined as Fields.FacetData) and let Servant serialise it. Returning a naked Value weakens the API contract unnecessarily and forces the filtered restructuring inside the handler rather than in the type. If the narrowing-by-field is the only reason for the Value return, a small FilteredFacets newtype (or even just AE.Object KeyMap) would be better.

4. findBin duplicates getE2EConfig internals (test/integration/CLI/CLIE2ESpec.hs)

findBin :: IO FilePath
findBin = T.unpack . T.strip . toText <$> readProcOut "cabal" ["list-bin", "exe:monoscope"]

getE2EConfig already resolves the binary path into E2EConfig.binPath. findBin is a separate entry point only used by the server-free auth/completion tests. The cabal list-bin call is invoked redundantly. Simplest fix: extract the bin-lookup into a one-time beforeAll or pass cfg.binPath through withReachableServer.

5. shouldHaveKeys — fromString . toString roundtrip (test/integration/CLI/CLIE2ESpec.hs)

KM.member (fromString (toString k)) obj

Text → String → Key when Data.Aeson.Key.fromText :: Text -> Key exists and is shorter. Add import Data.Aeson.Key qualified as AEK and use KM.member (AEK.fromText k) obj.

6. withCardFrame Bool parameter — weakly typed

withCardFrame :: Bool -> Widget -> Maybe (Text, Text) -> Html () -> Html ()

Bool arguments are hard to read at call sites (withCardFrame True widget …). A two-constructor type or a newtype FlexCol = FlexCol | NoFlex would make intent clearer without extra lines. Alternatively, two specialised helpers (withCardFrameFlex / withCardFrameFixed) since there are only three call sites. Minor, but the project clearly prefers legibility.

7. Repeated when (code /= ExitSuccess) $ expectationFailure in CLIE2ESpec

This idiom appears ~10 times. A small helper:

assertSuccess :: (ExitCode, String, String) -> IO ()
assertSuccess (ExitSuccess, _, _) = pure ()
assertSuccess (code, _, err) = expectationFailure $ "exit=" <> show code <> " stderr=" <> err

would make the tests more readable and the duplication disappears.

Minor nits

• widget.naked == Just True inside withCardFrame — fromMaybe False widget.naked is more idiomatic.
• addRespHeaders "" → addRespHeaders mempty was applied inconsistently; some "" calls in Settings.hs were converted, a few in Projects.hs billing section remain as "" rather than mempty.

Code Review — PR #365: Refactor/loc reduction cross cutting

Overview

This is a large, high-quality refactor touching the CLI, server pages, and shared models. The primary themes are:

1. CLI agent-mode support — stable JSON envelopes, --agent/MONOSCOPE_AGENT_MODE, --debug, new facets command, --first/--id-only, --cursor, etc.
2. mkPageCtx extraction — removes ~5 lines of boilerplate per page handler (≈25 callsites)
3. Deriving wins — WrappedEnumSC replaces manual FromJSON/ToJSON instances; deriving-aeson CamelToSnake/Rename replaces hand-written instances
4. CLI.Validate — clean new module; doctests are a good touch

The net LOC reduction is substantial and the code is generally more correct than what it replaces. Observations below are ordered by impact.

Good changes worth calling out

• hoistEither replacing ExceptT . pure — idiomatic and correct; Relude re-exports it.
• pass replacing pure () — consistent with the Relude idiom throughout the codebase.
• fromRight emptyConfig <$> … — simpler than either (const emptyConfig) id.
• one (k, v) instead of MapS.singleton k v — one from Relude is exactly right here.
• $> replacing >> pure in applyOne — strict improvement.
• unless replacing when … not in Anomalies.hs.
• withCardFrame — good extraction; renderLogsWidget/renderTraceTable/renderTable were nearly identical shells.
• SubPortalResponse / SubResponse via Rename "dataVal" "data" — replaces manual parseJSON instances cleanly.

Issues and suggestions

1. getField = toText . map toLower . drop 2 . show in Models/Apis/Fields.hs

instance HasField "toText" FieldTypes Text where
  getField = toText . map toLower . drop 2 . show

This replaces the explicit fieldTypeToText function with a show-based hack. The constructor names are FTString, FTBool, etc., so drop 2 . show gives "String", "Bool" — but the old encoding was "string", "bool" (lowercase). The map toLower handles that, but this is now tied to the exact constructor name spelling. If a constructor is ever renamed (e.g. FTUnknown → FTUndefined), the serialized form silently changes too. The old explicit mapping was more intentional about what the wire format was. At minimum add a doctest or note the invariant.

2. validateDurationFor doesn't strip before checking T.null

validateDurationFor flag t
  | T.null t = Left $ flag <> " must not be empty"
  | otherwise =
      let trimmed = T.strip t

The doctest shows validateDurationFor "--since" "" → Left. But " " (spaces only) would pass the T.null t guard, then T.strip produces "", and T.span isDigit "" gives ("", "") → T.null digits → Left with the wrong message ("must match...") instead of "must not be empty". Consider | T.null (T.strip t) = Left ....

3. withTraceSummary uses fromMaybe "" for tid then filters not (T.null tid)

, let tid = fromMaybe "" (cell "context.trace_id" r <|> cell "trace_id" r)
, not (T.null tid)

This is fine, but the guard could be let tidM = cell "context.trace_id" r <|> cell "trace_id" r, then only pattern-match on Just tid — avoids constructing the empty string at all:

[ (tid, ...)
| r <- d.rows
, Just tid <- [cell "context.trace_id" r <|> cell "trace_id" r]
]

4. renderParams logs unencoded query params to stderr — security note

renderParams ps = "?" <> T.intercalate "&" [k <> "=" <> v | (k, v) <- ps]

The comment correctly warns about credentials. Since the KQL query param can contain user-supplied strings, and MONOSCOPE_DEBUG is easy to accidentally enable in CI, consider truncating long values or at least noting this in the comment for future maintainers (e.g. query could be arbitrarily long).

5. normalizeListE — cursor: null hardcoded for Paged responses

, "cursor" AE..= AE.Null

If Paged ever gains a cursor field, this silently omits it. Minor, but worth noting since the stable envelope contract is the whole point of this PR.

6. filterSchema uses Map.take which is key-ordered, not count-ordered

capped = maybe filtered (`Map.take` filtered) opts.schemaLimit

Map.take n takes the first n keys in ascending key order. This is fine for a --limit that's meant to reduce context window size, but if users expect the "most important" fields first, alphabetical ordering may surprise them. The docstring says "Cap on returned fields (post-filter)" — just worth verifying the expectation is clear in the help text.

7. Blank lines between top-level bindings

Multiple places add a blank line after every top-level binding:

data Foo = ...
  deriving ...


bar :: ...
bar = ...


baz :: ...

The project previously used single blank lines. This PR introduces double blank lines throughout CLI/Commands.hs and CLI/Core.hs. Not a bug, but inconsistent with the rest of the codebase — worth picking a convention and staying consistent.

8. dashboardGetH still calls ask @AuthContext after switching to mkPageCtx

dashboardGetH pid dashId fileM ... = do
  (_, project, bw) <- mkPageCtx pid
  appCtx <- ask @AuthContext          -- ← still present
  ...
  widgetsWithPngUrls <- populateWidgetPngUrls appCtx.env.apiKeyEncryptionSecretKey bw.config.hostUrl ...

appCtx.env.apiKeyEncryptionSecretKey is still used, so the ask is needed — but bw.config.hostUrl is also used in the same call. This mixes appCtx.env and bw.config which presumably refer to the same config. If bw.config == appCtx.config, then appCtx.env.apiKeyEncryptionSecretKey could just be bw.config.apiKeyEncryptionSecretKey (or similar), removing the residual ask. Same pattern appears in dashboardTabGetH. Not blocking, but worth a follow-up cleanup.

9. Env var rename MONO_* → MONOSCOPE_* is a breaking change

The rename is well-motivated, but existing users who have MONO_API_KEY set in their CI/shell will silently stop authenticating after upgrading. The docs don't mention this as a migration note (only the JSON output shape change is called out in docs/cli.md). Consider adding a migration note or a deprecation fallback that reads the old env vars with a warning.

Minor / nits

• CLI/Commands.hs:180: ExceptT $ liftIO $ withSpinner ... — the formatting change ($ chained vertically) is correct but reads slightly awkwardly; fine as-is.
• CLI/UI.hs: T.strip . unlines — unlines here is the Relude version which operates on [Text]; consistent, good.
• withAsync spinLoop (const action) — replacing withAsync spinLoop \_ -> action with \_ -> was the original; const is equally readable.
• Monitors.hs teamAlertsGetH: switches to _ <- Projects.sessionAndProject pid (no mkPageCtx) since it doesn't need a BWConfig — correct.

Test coverage

The new CLI.CLIE2ESpec is the right approach — white-box end-to-end tests that validate wire format rather than mocks. The CLIBinarySpec helpCases requirement in the docs is excellent.

One gap: CLI.Validate doctests won't run until the module is added to a doctest suite. The docs mention this but it's worth tracking as a follow-up.

Overall this is a solid, well-motivated PR. The agent-mode infrastructure is clean, the page-handler boilerplate reduction is significant, and the derive-based JSON instance consolidation removes a real class of drift bugs. The issues above are mostly minor and none are blockers.

Code Review: Refactor/loc reduction cross cutting

Good overall direction — the agent-mode hardening, mkPageCtx abstraction, toastError combinator, deriving-aeson cleanup, and normalizeListE are solid wins. Notes below are mostly about tightening what's here rather than anything structurally wrong.

⚠️ Type safety: two booleans where a sum type belongs

EventsSearchOpts gains two mutually exclusive flags:

, firstOnly :: Bool   -- --first
, idOnly :: Bool      -- --id-only

These should be a sum type — the compiler can't enforce mutual exclusion on two bools, and adding a third output mode later will require another bool:

data EventsOutputScope = AllEvents | FirstOnly | IdOnly

data EventsSearchOpts = EventsSearchOpts
  { ...
  , outputScope :: EventsOutputScope
  }

--id-only already implies --first; the current guard let firstOnly = validatedOpts.firstOnly || validatedOpts.idOnly is the smell.

Duplicate block in AuthStatus

The human-mode output for "env" and "token" is identical except for the first line:

Just "env" -> do
  putTextLn "Authenticated via MONOSCOPE_API_KEY environment variable"
  putTextLn $ "API URL: " <> cfg.apiUrl
  whenJust cfg.projectId $ \p -> putTextLn $ "Project: " <> p
Just "token" -> do
  putTextLn "Authenticated via stored token"
  putTextLn $ "API URL: " <> cfg.apiUrl
  whenJust cfg.projectId $ \p -> putTextLn $ "Project: " <> p

Extract a helper to DRY this up:

let printStatus label = do
      putTextLn label
      putTextLn $ "API URL: " <> cfg.apiUrl
      whenJust cfg.projectId $ \p -> putTextLn $ "Project: " <> p
case method of
  Just "env"   -> printStatus "Authenticated via MONOSCOPE_API_KEY environment variable"
  Just "token" -> printStatus "Authenticated via stored token"
  _            -> printError "Not authenticated. Run: monoscope auth login --token <token>"

Magic number in apiFacets

defaultFrom = fromMaybe (addUTCTime (-86400) now) fromT

Use nominalDay from Data.Time (already in your dependency graph):

import Data.Time (nominalDay)
defaultFrom = fromMaybe (addUTCTime (negate nominalDay) now) fromT

capFacets: redundant first clause

capFacets Nothing v = v
capFacets (Just n) (AE.Object obj) = ...
capFacets _ v = v          -- catches Just + non-Object

The first clause is subsumed by the third. Simplify to:

capFacets (Just n) (AE.Object obj) = AE.Object $ flip KM.map obj $ \case
  AE.Array xs -> AE.Array (V.take n xs)
  other -> other
capFacets _ v = v

parseDurationMs not case-insensitive

validateDurationFor accepts "1H" / "24H" (uppercase, as emitted by the TimePicker). parseDurationMs only strips lowercase suffixes, so --watch 5M passes validation but resolves to the 5000ms fallback silently. Either normalise to lowercase before stripping, or call validateDurationFor first.

Data.List (isInfixOf) in CLIE2ESpec.hs

import Data.List (isInfixOf)

Relude already re-exports isInfixOf. Drop the explicit import.

reqOpts operator alignment (formatter artifact)

reqOpts cfg params =
  W.defaults
    & W.header "Accept"
    .~ ["application/json"]
      & addAuth cfg.apiKey
      & addProjectId cfg.projectId
      & addParams params

The & addAuth / & addProjectId are indented under .~ rather than aligning with the first &. Should be:

reqOpts cfg params =
  W.defaults
    & W.header "Accept" .~ ["application/json"]
    & addAuth cfg.apiKey
    & addProjectId cfg.projectId
    & addParams params

normalizeListE silent fallback in runList

When the shape is unrecognised, the raw value is passed through with only a printDebug message (invisible without --debug):

Left (raw, reason) -> do
  printDebug $ "list " <> resourcePath k <> ": " <> reason
  renderByMode mode Nothing raw

Agents relying on {data, pagination} get a broken shape with no indication anything went wrong. Consider printError (not printDebug) for the reason here — it's an unexpected server shape, not routine debug noise.

DecodedEvents.raw stores the full object just for three lookups

data DecodedEvents = DecodedEvents
  { idxMap :: Map Text Int
  , rows   :: [[Text]]
  , raw    :: KM.KeyMap AE.Value   -- used only for hasMore, cursor, count
  }

If you decode those three fields at construction time you avoid threading a raw KeyMap through the API and the intent is clearer:

data DecodedEvents = DecodedEvents
  { idxMap  :: Map Text Int
  , rows    :: [[Text]]
  , count   :: Maybe Int
  , hasMore :: Maybe Bool
  , cursor  :: Maybe Text
  }

Minor nits

• isAgentMode / isDebugMode each re-read env vars on every call; no structural problem but noting it in case hot paths hit these repeatedly.
• Data.Char (toLower) import in Core.hs — Relude re-exports toLower via Data.Char; the explicit import is harmless but redundant.
• ExceptT . pure → hoistEither improvement in runAuth is a nice use of the existing utility; good pattern to apply if any other ExceptT . pure remain.

Code Review: Refactor/loc-reduction cross-cutting

Overview

This is a substantial PR adding agentic-mode improvements to the CLI (structured JSON output, pagination, facets command, --debug/--first/--id-only flags), fixing long-standing bugs (KQL operator, MONO_* env var rename, auth login in agent mode), and reducing page-handler boilerplate via mkPageCtx/toastError/withCardFrame. The structural direction is good. Notes below, roughly by priority.

Potential bugs

1. map NE.head to map head in Anomalies.hs

NE.groupBy returns [NonEmpty a]. NE.head :: NonEmpty a -> a is total and preserves the type. Relude's head :: [a] -> Maybe a does not match NonEmpty a — this should be a type error, or if Relude exports a Foldable-polymorphic variant, changes the result type to [Maybe Breadcrumb], which would fail downstream. Please verify this compiles and has the intended semantics.

2. decodeEvents succeeds on any Object — masking server errors

decodeEvents returns Just DecodedEvents{...} for any JSON Object using mempty/0/AE.Null defaults for missing fields. A server error response like {"error": "KQL parse failed"} silently decodes as zero events rather than surfacing the error. Either return Nothing when mandatory keys (logsData, colIdxMap) are absent, or document that this is intentional.

3. httpExToError body is untruncated

The body is now included verbatim in error messages — great for KQL parse errors, but a WAF/CDN HTML error page could produce kilobytes on stderr. Add T.take 500 bodyTxt before the final msg assembly.

Package / extension opportunities

4. UUID validation reimplements uuid functionality

validateUuid manually parses 8-4-4-4-12 hex. The uuid package (already in the project) exports UUID.fromText :: Text -> Maybe UUID. Use it:

validateUuid t
  | T.null t = Left "expected UUID; got empty string"
  | isNothing (UUID.fromText t) = Left $ "expected UUID (8-4-4-4-12 hex); got '" <> t <> "'"
  | otherwise = Right t

5. FieldTypes HasField "toText" via show is fragile

toText . map toLower . drop 2 . show assumes every constructor starts with exactly "FT". A constructor added without that prefix breaks this silently. The WrappedEnumSC "FT" deriving already handles ToJSON/FromJSON correctly; the HasField instance should use the same mechanism or have a unit test that round-trips every constructor.

6. Three Main.hs callsites still use the silent normalizeList

CLI/Resource.hs documents: "Prefer normalizeListE from new code so the caller can flag unexpected payloads." But three places in Main.hs use the silent fallthrough:

Resource.normalizeList <<$>> apiGetJson ... -- issues, endpoints, log-patterns

These should go through runList (which calls normalizeListE) so a shape mismatch produces the stderr warning.

Conciseness / style

7. Double blank lines add ~100 lines of noise

The PR introduces double blank lines between most top-level declarations in Commands.hs, Core.hs, Resource.hs, etc. Standard GHC/Haskell style uses single blank lines. This inflates the diff without adding information.

8. Audit-finding codes in comments will rot

Codes like -- C7:, -- D5:, -- B9: reference an internal audit document future maintainers won't have. Keep the behavioural description, drop the opaque code, or link to the PR.

9. apiFacets computes the time default twice

TP.parseTimeRange returns (from, to, _), and the _ discards the range. The subsequent fromMaybe (addUTCTime (negate nominalDay) now) fromT duplicates the 24h default logic that parseTimeRange would apply if passed since = Just "24h". Pass the default directly and drop the extra fromMaybe.

Migration note (breaking change)

The MONO_* to MONOSCOPE_* rename will silently break existing CI configs and dotfiles. Consider printing a one-time warning to stderr when a MONO_* var is set but its MONOSCOPE_* counterpart is not, so users get a clear signal rather than silent auth failure.

What's good

• mkPageCtx / toastError / withCardFrame collapse real repetition cleanly.
• hoistEither, maybeToRight, $>, pass, one substitutions are all correct idiomatic improvements.
• normalizeKind + validateKind separation is clean; validateAndNormalizeKind makes callers concise.
• Doctest coverage in CLI/Validate.hs is a lightweight but effective regression guard.
• CLIE2ESpec.hs is well-structured: per-finding assertions, graceful pending when env vars are absent, clear withReachableServer helper.
• emitCompletion failing loudly for unknown shells is a clear improvement.
• isAgentMode truthy check (rejecting "false"/"0") avoids surprising activation from dev-machine CI exports.
• fromRight emptyConfig, hoistEither, uncurry LPList, T.splitOn "," <$> — all nice uses of existing functions over manual lambdas.

Code Review — Refactor/loc reduction cross cutting

Good overall direction: adding agent-mode capabilities, normalising JSON envelopes, and cleaning up cross-cutting concerns. Several concrete issues worth addressing before merge.

Breaking change (no migration path)

MONO_* → MONOSCOPE_* renames every env var users already have in ~/.bashrc, CI secrets, and docker-compose files. There is no deprecation shim — existing integrations silently break on upgrade. Either keep reading the old names as a fallback (printing a deprecation warning) or call this out prominently in the PR description with a migration guide.

Bug: backslash not escaped in foldFiltersIntoQuery

-- cli/CLI/Commands.hs
let q v = "\"" <> T.replace "\"" "\\\"" v <> "\""

Quotes are escaped but backslashes are not. A service name like foo\nbar produces "foo\nbar" in KQL, which the parser may interpret as an escape sequence. Compare with runEventsGet, which correctly escapes \\ first:

let eid = T.replace "\"" "\\\"" (T.replace "\\" "\\\\" opts.eventId)

Apply the same two-step escape to q.

Fragile HasField "toText" implementation

-- src/Models/Apis/Fields.hs
getField = toText . map toLower . drop 2 . show

drop 2 hard-codes the "FT" constructor prefix. This silently produces wrong output if a constructor is renamed or a new one starts with a different prefix. WrappedEnumSC already provides display that maps constructors correctly via the type-level prefix — use that instead:

getField = display  -- WrappedEnumSC "FT" already strips and lowercases

Internal audit codes in comments

Throughout Commands.hs, Core.hs, and Main.hs there are inline codes like -- C7:, -- C8:, -- D5:, -- C4:, -- C11:, -- B9:, -- C12:. These are meaningless to future readers — they reference an external review document that won't travel with the code. Strip them; the prose explanation after the colon can stand alone or belong in the commit message.

Comment density

Several functions accumulate multi-paragraph Haddock blocks explaining implementation history and prior iterations (normalizeDecoded, withTraceSummary, buildSearchParams, normalizeListE). The project convention is one short line for non-obvious WHY, nothing for obvious WHAT. The history ("Earlier iterations went via /api/v1/metrics (returned empty…)") belongs in the commit message, not in the source.

Minor points

map head vs map NE.head (Anomalies.hs)

- deduped = map NE.head $ NE.groupBy ((==) `on` dedupKey) $ sortOn dedupKey raw
+ deduped = map head   $ NE.groupBy ((==) `on` dedupKey) $ sortOn dedupKey raw

NE.groupBy returns [NonEmpty a], so NE.head is the right, total accessor. Relude's head :: NonEmpty a -> a is also safe here, but the explicit NE.head is clearer about the invariant being exploited.

extractColIdxMap uses round

Map.fromList [(AK.toText k, round n) | (k, AE.Number n) <- KM.toList obj]

Column indices are always integers from the server. truncate or floor communicates intent better than round (which is surprising on a half-value) and fromIntegral . floor would avoid the RealFrac constraint.

setEnv in main
The comment correctly notes the race is empty here. Fine as-is, but consider checking global.debugFlag || isJust (lookupEnv "MONOSCOPE_DEBUG") in isDebugMode directly rather than mutating the environment — avoids surprising any future concurrent tests that fork main.

normalizeListE Left branch in runList
The fallback prints to stderr and still renders the raw value. Good. The message could mention the raw shape (e.g. "Object with keys: ...") to help diagnose server-side envelope changes without having to rerun with --debug.

What works well

• CLI.Validate module: clean separation with doctests, good validateOrDie combinator.
• hoistEither replaces the verbose ExceptT . pure.
• isAgentMode truthy check (empty / "false" / "0" → off) is exactly right for CI=false environments.
• withTraceSummary / DecodedEvents sharing a single parse pass is the correct approach for large payloads.
• normalizeListE returning Either (Value, Text) so callers can decide whether to warn is a good API design.
• mkPageCtx eliminating the (sess, project) / ask @AuthContext / def{...} boilerplate across handlers is a clear win.
• pass replacing pure () and $> replacing >> pure throughout are idiomatic Relude improvements.

Code Review — PR #365 (Refactor/LOC reduction cross cutting)

Overall this is a solid, well-motivated refactor with real functional improvements bundled in. The LOC reduction is genuine and the agent-mode additions are thoughtfully designed. Below are specific findings, roughly prioritised.

Bugs / correctness issues

isAgentMode — CI falsy-value logic is correct but brittle in context
CLI/Core.hs — the new truthy correctly rejects CI=false / CI=0, but CI is also checked in isInteractiveTTY via isAgentMode. A shell that exports CI=false (common in direnv setups) will correctly pass through, but the comment says "many dev machines export CI unconditionally" and only mentions it for direnv/IDE — worth a note that CI=false is explicitly handled so future readers don't "fix" it.

getField on FieldTypes uses show + string manipulation
src/Models/Apis/Fields.hs:91:

getField = toText . map toLower . drop 2 . show

show FTUnknown → "FTUnknown" → drop 2 → "Unknown" → "unknown". This works for every current constructor (all prefixed exactly "FT") but silently breaks if a constructor is added without the FT prefix. The old fieldTypeToText was explicit and total. The WrappedEnumSC "FT" FieldTypes via-deriving already knows the prefix — consider using display (or getField @"toText" from the derived instance) rather than re-implementing it with show. At minimum add a {-# DEPRECATED #-} comment or a test.

extractColIdxMap uses round for Double → Int
CLI/Commands.hs — round n on a Scientific that arrived from JSON is fine in practice (the server sends integers), but floor or fromIntegral . truncate would communicate intent more clearly and not silently round 1.9 → 2.

withTraceSummary falls through silently on non-Object values
CLI/Commands.hs:

withTraceSummary _ v = v

If the events call returns something other than an Object (e.g. unexpected server shape change), --summary silently produces output with no traces key. A printDebug here (matching the pattern in withMetricsData) would at least surface it under --debug.

Code quality / conciseness opportunities

validateDurationFor — can use T.break (not . isDigit)

-- current
let (digits, suffix) = T.span isDigit trimmed
-- same, more idiomatic with Relude

This is fine, but T.null digits check + T.toLower suffix \notElem`can be expressed as a single pattern match using the already-importedisDigit`. Minor, no change needed.

normalizeListE exposes Left (AE.Value, Text) but callers only use the Text
CLI/Resource.hs — runList prints reason and falls through to raw. But the type carries both so callers could also render raw themselves. The API is fine for now, but the Left tuple makes the type slightly awkward; an Either (AE.Value, Text) AE.Value could be Either Text AE.Value if raw were always the input v. Small ergonomics issue.

toastError pattern — could be a single helper everywhere
Pages/Projects.hs, Pages/Dashboards.hs etc. consistently do:

addErrorToast msg Nothing >> addRespHeaders (SomeError msg)

The new toastError helper in System.Types is used in some places but not all — Pages/Monitors.hs still has addErrorToast e Nothing >> addReswap "" >> addRespHeaders (ManageTeamsPostError e) which can't collapse (needs addReswap), but Dashboards.hs line ~1768 (toastError "Dashboard title is required" ...) is now consistent. Worth checking any remaining addErrorToast msg Nothing >> addRespHeaders patterns that weren't converted.

mkPageCtx — (sess, project, bw) pattern with _ on sess
Most callers do (_, project, bw) <- mkPageCtx pid or (_, _, bw) <- mkPageCtx. The sess field goes unused in ~70% of call sites. Consider whether sess should be in the tuple at all, or whether a separate bwOnly variant makes the intent clearer. The current API works but the ignored binding adds noise.

buildSearchParams — sinceParam could use bool

-- current
sinceParam = case opts.since of
  Just s -> s
  Nothing -> if isJust opts.from || isJust opts.to then "" else "1H"
-- more point-free
sinceParam = fromMaybe (bool "1H" "" (isJust opts.from || isJust opts.to)) opts.since

Module export list style — inconsistent trailing comma style
The PR converts some modules from ( x, y ) style to ( x, y, ) trailing-comma style (CLI/Commands.hs, CLI/Core.hs, CLI/Resource.hs). CLI/Validate.hs (new file) still uses the old ( x\n, y\n) style. Pick one and apply consistently.

Blank lines between top-level definitions
Many single blank lines were added between every top-level binding in CLI/Commands.hs and CLI/Core.hs. Project convention appears to be no separator between closely related helpers and a single blank between unrelated ones. The added blanks after every single definition (including one-liners like renderSummaryCell, extractColIdxMap) adds visual noise. Worth a quick pass with fourmolu/ormolu to settle this.

Performance

normalizeDecoded builds keep list on every call
CLI/Commands.hs — keep = Map.keys d.idxMap allocates a list of all column names; this is called once per response so it's not a hot path, but could be toList directly.

withTraceSummary uses Map.fromListWith with set union
(s1 <> s2, ...) on Set Text inside fromListWith is O(n log n) for each merge — fine for real-world event counts (hundreds, not millions).

Security

KQL injection in runEventsGet
The double-quote and backslash escaping is correct and necessary — good catch. One edge case: the constructed query (id==\"...\") or (context.trace_id==\"...\") uses or — if the server's KQL parser precedence is and > or (standard), this is fine. Worth a note or a test to confirm the parenthesisation is correct.

renderParams logs raw query params to stderr including KQL
CLI/Core.hs:363 — the comment already says "values are NOT URL-encoded so KQL characters are readable" and warns about credentials in query params. However the query param can contain user-supplied KQL that might include sensitive data (e.g. attributes.db.statement==\"SELECT * FROM users WHERE password='...'). This is debug-only and opt-in (MONOSCOPE_DEBUG=1) but worth noting in the comment that the query value may contain data from the user's telemetry.

MONOSCOPE_DEBUG=1 set via setEnv in Main.hs
The comment correctly notes setEnv is not thread-safe but says the race window is empty because it's set before runCLI. This is sound. Good.

Test coverage

The new CLI/CLIE2ESpec.hs and CLI/CLIBinarySpec.hs additions are a significant improvement. A few gaps:

• foldFiltersIntoQuery has doctests — great. buildSearchParams and takeFirstEvent/takeFirstRow do not. These are pure functions with branching logic that previously had bugs; doctests would cement the fix.
• normalizeListE has no tests. The three-shape recognition (already-normalised, Paged, bare array) is non-trivial and the right place for a unit test.
• withTraceSummary is tested indirectly via E2E; a unit test with a known JSON fixture would be cheaper to maintain.

Minor

• CLI/UI.hs: pure () → pass is idiomatic Relude — nice.
• currentValue = T.strip . unlines . BE.getEditContents — T.unlines vs unlines: unlines here is Prelude.unlines :: [String] -> String because BE.getEditContents :: Editor t n -> [t] and t ~ Text... actually unlines on [Text] would require Relude's unlines which works on [Text]. Verify this compiles cleanly (it should with Relude's unlines).
• BackgroundJobs.hs: CE.ErrorCall — the qualified import avoids the wildcard, good.
• MapS.singleton → one in Telemetry.hs — nice use of Relude.one.
• NE.head → head in Anomalies.hs — fine since NE.groupBy returns [NonEmpty a] and head on a NonEmpty is safe here, but NE.head would be slightly clearer about why head can't fail.

Summary

The core refactors (mkPageCtx, AuthStatusJson/ConfigGetJson via deriving-aeson, normalizeListE, withMetricsData) are all improvements. The env-var rename (MONO_* → MONOSCOPE_*) is a breaking change for existing users — ensure it's called out prominently in release notes and/or a migration guide. The FieldTypes.getField via show-manipulation deserves another look before merging.

Code Review — Refactor/LOC reduction cross-cutting

Good overall direction. The PR eliminates a lot of boilerplate (mkPageCtx, normalizeList, withMetricsData, filterSchema) and fixes real bugs (wrong KQL operator : → ==, broken trace_id: lookup, missing Environment constraints). Below are concrete improvement opportunities, ordered roughly by impact.

Bugs / correctness

withTraceSummary leaks empty-trace-id rows as a blank key

groups = Map.fromListWith merge
  [ (tid, …)
  | r <- d.rows
  , let tid = fromMaybe "" (cell "context.trace_id" r <|> cell "trace_id" r)
  , not (T.null tid)   -- ← this guard already removes them
  ]

The guard is present, so this is fine — but the comment on L648 says "Empty trace_ids are dropped" only in traces, not in groups. The guard is in groups, not traces. The traces list then calls filter (not . T.null) on services but not on trace ids. This is consistent but the comment is misleading — minor.

isAgentMode uses map toLower on a String

truthy = maybe False (\v -> v /= "" && map toLower v `notElem` ["false", "0"])

v is a String (from Env.lookupEnv), so map toLower is correct. But Relude re-exports Data.Char.toLower — no issue here, just noting that Data.Char (toLower) import is added explicitly which is redundant since Relude already exports it. Can drop the import.

emitFirstEventId exits non-zero but prints to stderr via printError; printError writes "error: …" which is correct, but --id-only is a deliberate empty-result case an agent might want to handle gracefully. Consider exiting 1 only when the envelope itself is malformed and exiting 0 with empty output when events is genuinely empty — distinguishes "no matches" from "bad response". Not a blocker, just a UX note for agents.

Succinctness / use existing packages

takeFirstEvent / takeFirstRow — use Lens

Both functions pattern-match on AE.Object, look up a key, modify a sub-array with V.take 1, and re-insert. The project already has Data.Aeson.Lens:

-- before (14 lines for two functions)
takeFirstEvent (AE.Object obj) = case KM.lookup "events" obj of
  Just (AE.Array arr) -> AE.Object (KM.insert "events" (AE.Array (V.take 1 arr)) obj)
  _ -> AE.Object obj
takeFirstEvent v = v

-- after (4 lines, same semantics)
takeFirstEvent = key "events" . _Array %~ V.take 1
takeFirstRow   = key "logsData" . _Array %~ V.take 1

foldFiltersIntoQuery — the case (T.null prefix, T.null trimmed) can use bool/guards

-- current
case (T.null prefix, T.null trimmed) of
  (True, _) -> trimmed
  (_, True) -> prefix
  _         -> prefix <> " and (" <> trimmed <> ")"

-- more idiomatic with Relude's `guarded` or just pattern guards
foldFiltersIntoQuery query mService mLevel
  | T.null prefix = trimmed
  | T.null trimmed = prefix
  | otherwise = prefix <> " and (" <> trimmed <> ")"
  where …

normalizeListE — prefer has (key "data") v && has (key "pagination") v (aeson-lens) over AEKM.member "data" obj && AEKM.member "pagination" obj. The project already imports Data.Aeson.Lens elsewhere.

extractColIdxMap — round on Scientific loses the type annotation

Map.fromList [(AK.toText k, round n) | (k, AE.Number n) <- KM.toList obj]

round n :: Int works, but floor (or truncate) is more intentional here since column indices are always non-negative integers that come from the server as exact integers. Use floor to make the intent explicit.

parseDurationMs view pattern

parseDurationMs (T.toLower -> t)

Good use of ViewPatterns. The fallthrough | otherwise = maybe 5000 (* 1000) (readMaybe $ toString t) silently treats a bare number as seconds, which is surprising — should it error/exitFailure instead? (Consistent with validateDurationOrDie which rejects non-suffixed values.)

withTraceSummary's cell helper is a local closure re-derived every call

cell :: Text -> [Text] -> Maybe Text
cell field r = Map.lookup field d.idxMap >>= \i -> listToMaybe (drop i r)

Map.lookup field d.idxMap is the same for a given field regardless of row. Pre-compute the index:

svcIdx  = Map.lookup "service" d.idxMap <|> Map.lookup "resource.service.name" d.idxMap
errIdx  = Map.lookup "errors"  d.idxMap
tidIdx  = Map.lookup "context.trace_id" d.idxMap <|> Map.lookup "trace_id" d.idxMap
at idx r = idx >>= \i -> listToMaybe (drop i r)

Saves a Map.lookup per cell per row. On large events context payloads this matters.

FieldTypes.getField — fragile show-based implementation

instance HasField "toText" FieldTypes Text where
  getField = toText . map toLower . drop 2 . show

This relies on the Show instance generating "FTString", "FTNumber", etc. — which is true for deriving stock Show but is an invisible invariant. If anyone adds deriving (Show) via SomeWrapper or adds a constructor that doesn't start with "FT", the output silently changes. The old fieldTypeToText was explicit pattern-match; the new version saves 7 lines but introduces a hidden dependency on the constructor naming convention. At minimum, add a note, or use a WrappedEnumSC-style approach that's already used for FromField/ToField.

setEnv in Main.hs

when global.debugFlag $ setEnv "MONOSCOPE_DEBUG" "1"

The comment acknowledges this is process-global and not thread-safe. That's fine here, but System.Environment.setEnv is not available in the Effectful.Environment effect — you're dropping to raw IO. Consider Env.setEnv from effectful-core if it's available, so the environment effect layer is consistent. Minor.

mkPageCtx type — partial use of sess

Many callers pattern-match (_, project, bw) — they never use sess. Consider returning (Project, BWConfig) and providing a separate mkPageCtxWithSession for the handful of handlers that need sess. This would make it obvious at the call site that sess is unused (currently GHC warns, callers silence with _).

Env var rename (MONO_* → MONOSCOPE_*) — migration note missing

The docs are updated, but there's no deprecation shim or transition period noted. Users with MONO_API_KEY in their dotfiles will silently lose authentication. A one-release "check old name and warn" path would be friendlier.

Minor / formatting

• pure () → pass in CLI/UI.hs — good use of Relude.
• withAsync spinLoop \_ -> → withAsync spinLoop (const action) — cleaner.
• fmap (T.splitOn ",") $ → T.splitOn "," <$> — good.
• uncurry LPList <$> pageOpts — good.
• Blank lines added between top-level declarations throughout — keeps diff noisy but aids readability consistently.
• (IOE :> es) => → IOE :> es => (dropped parens on single-constraint) — consistent with project style.

Test coverage

CLI.CLIE2ESpec is a great addition. A few gaps worth noting:

• No test for --id-only returning exit 1 on empty results.
• No test for the normalizeListE fallthrough path (unknown envelope shape emits stderr warning).
• The validateDurationFor doctests won't run until the module is added to the doctest suite — wire that up or they provide false assurance.

Summary: The refactoring is solid and the bug fixes are real. The main improvements available are: use aeson-lens for the takeFirst* helpers (big LOC reduction), pre-compute column indices in withTraceSummary, and add a deprecation path for MONO_* env vars.

Code Review: CLI agent-mode hardening + cross-cutting LOC reduction

Overview

Solid PR. The KQL operator fix (== replacing :) is the most impactful correctness change — it unblocked every --service/--level flag and events get. The mkPageCtx extraction, toastError, withCardFrame, and deriving-aeson migrations all cut real LOC with no loss of clarity. The normalizeListE + stable JSON envelope work is the right foundation for agent-mode clients.

Potential bugs / issues

1. foldFiltersIntoQuery escapes " but not \ in service/level values

q v = "\"" <> T.replace "\"" "\\\"" v <> "\""

A service name containing a backslash (e.g. "my\\service") produces resource.service.name=="my\service" — valid JSON but invalid KQL (dangling escape). Compare with runEventsGet, which correctly double-escapes:

let eid = T.replace "\"" "\\\"" (T.replace "\\" "\\\\" opts.eventId)

Apply the same pattern in q:

q v = "\"" <> T.replace "\\" "\\\\" (T.replace "\"" "\\\"" v) <> "\""

2. HasField "toText" FieldTypes via drop 2 . show is fragile

instance HasField "toText" FieldTypes Text where
  getField = toText . map toLower . drop 2 . show

This silently breaks if any constructor is renamed to one without a 2-char prefix (e.g. FTUnknown → Unknown). Since WrappedEnumSC "FT" already handles the serialisation prefix for DB/JSON, consider deriving toText from the same mechanism or adding a test that each constructor round-trips through toText.

3. runEventsGet OR query for single-event lookup

"(id==\"" <> eid <> "\") or (context.trace_id==\"" <> eid <> "\")"

When --tree is false, this returns any event whose id or context.trace_id matches. Legitimate if you want to resolve a trace ID to its root span, but could return multiple rows if the ID collides. Worth a comment noting this dual-lookup intent.