[jess-nylas]

Summary

Adds nylas deliverability, a new command group for the SendEvents, TrackingEvents and send alert API routes (17 routes). The subgroups match the "Deliverability & Engagement" section of the API reference (developer.nylas.com#797):

Command	Subcommands	API routes
nylas deliverability send-events	list, show, message, summary, timeseries, breakdown	/v3/activity/send, /v3/metrics/send
nylas deliverability tracking-events	list, show, message, summary, timeseries, breakdown	/v3/activity/tracking, /v3/metrics/tracking
nylas deliverability send-alerts	list, show, create, update, delete	/v3/alerts/send

Jira: TW-8105 (epic TW-6497).

Why

The send and tracking event APIs are live in prod in US and EU, but the CLI had no way to read them or to manage send alert rules.

What changed

• Domain, port, adapter: internal/domain/deliverability_*.go, internal/ports/deliverability.go, internal/adapters/nylas/deliverability_*.go, plus mock and demo clients.
• Commands: internal/cli/deliverability/, registered in cmd/nylas/main.go.
• App-level auth: all commands use the API key through common.WithClientNoGrant. No command takes a grant; a grant token gets the server's 403.
• Activity list:
  • Repeatable --event-type and per-feed filters.
  • --start/--end accept Unix seconds, YYYY-MM-DD (a whole UTC day), YYYY-MM-DD HH:MM[:SS] and YYYY-MM-DDTHH:MM[:SS] (UTC), RFC3339, and look-backs (24h, 7d).
  • Words like today or 9am are rejected, because they resolve differently on each run and break --page-token.
  • --end alone defaults --start to 6 days before it. An --end older than the 90-day retention is rejected locally; otherwise the server returns an empty list with no error.
  • Single-page paging with --limit and --page-token. The page token is bound to the exact start and end, so a look-back can't be combined with --page-token. After a look-back, the CLI prints Next page: --page-token … --start <epoch> for the next page.
• Metrics: summary (health, totals, graded rate tiles, findings), timeseries and breakdown. A null rate prints — rather than 0%.
• send-alerts:
  • create sends --threshold as a JSON number; it is checked against the API's literal format first.
  • update sends only the flags you set (PUT with partial semantics).
  • delete needs --yes.
  • POST is not retried, so a timeout can't create a duplicate rule.
• Scope in the help: every command's --help states that it covers only Agent Account and Transactional send messages, not connected-grant (Google, Microsoft) sends. The wording matches the API reference. A test fails if a new subcommand leaves this out.
• Help clarity: show, message, update and delete say where their ID comes from and have examples. send-alerts create marks its required flags, update says unset flags keep their value, and delete --yes says it is required.
• Output formats: --format yaml uses the same keys and nesting as --json and the API (yaml tags mirror the json tags; embedded structs are inlined). Quiet mode follows the CLI-wide precedence, so --json --format quiet prints the JSON envelope.
• Docs: docs/commands/deliverability.md, plus docs/COMMANDS.md, INDEX.md, ARCHITECTURE.md and DEVELOPMENT.md. The deliverability page links to the developer docs guide, which ships in developer.nylas.com#797; the link 404s until that PR merges.

Testing

• Unit tests:
  • The adapter tests use httptest: exact query strings, request bodies, partial PUT, null rates, error envelopes.
  • The command tests run the real Cobra tree against a fake API.
  • Each new test was mutation-checked: I reverted the behaviour it guards and confirmed it fails.
• Full suite: go test -short ./... and race tests on the new packages pass.
• Lint: make lint fails locally, because the local golangci-lint is built with Go 1.24. I ran the CI-pinned version instead (GOTOOLCHAIN=go1.26.5 go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.1 run): 0 issues. make ci's other steps (fmt, vet, unit, race, security, vuln, build) pass.
• Integration tests on staging (internal/cli/integration/deliverability_test.go, API key only): the read commands, and an alert rule create → update → show → delete lifecycle. They pass. make ci-full was not run.
• Manual on staging:
  • All 17 routes were exercised.
  • send-events show --json is identical to curl of the same route.
  • Page 2 round trips work, including after a look-back.
  • Error paths: invalid cursor, bad dates, bad thresholds, server 400s.
• EU prod (read-only): summary and list with config set region eu.
• Code review: five /code-review high rounds ran on the branch; the fixes are in the later commits.

Notes

• Times are UTC on purpose. Deliverability prints 2026-10-29 16:13:55 UTC instead of the CLI's usual local common.Display* formats. That matches the API's UTC windows and metrics days, and a printed time can be pasted back into --start/--end.
• --quiet prints only IDs. To page from a script, use --json and read next_cursor.
• Known follow-ups, from the last review round, not blocking:
  1. A trailing UTC is accepted after any time value, including one with an RFC3339 offset.
  2. The parse error quotes the value after UTC is removed, not what the user typed.
  3. The 90-day boundary test doesn't check the end value that was sent.
  4. The demo message activity has its recipient list written out by hand.
  5. One demo test assertion overlaps with another.
  6. UTC repeats on every table row; a TIME (UTC) header would save width.

🤖 Generated with Claude Code

[qasim-nylas]

Code review of PR #129: Claude and Codex

Reviewed at 02223ce against main. Claude and Codex each reviewed the PR on their own, and every finding below was checked against the code. Where it was practical, the finding was also reproduced against a local fake API or a throwaway test.

Verdict: no blockers. The design is solid and the tests are strong. One finding is worth fixing before merge (it is one of your known follow-ups, but it is worse than described). The rest are small fixes that could go in this PR or a follow-up.

Build and tests

• Pass: go build ./....
• Pass: go vet on the domain, adapter and CLI packages, and on internal/cli/integration with -tags integration.
• Pass, 0 issues: golangci-lint run on the changed packages.
• Pass: go test -short on cli/deliverability, adapters/nylas and domain, and -race on the new tests.
• File sizes are fine: the largest file is deliverability_test.go at 458 lines, and no non-test file is over 248.

---

Should fix

1. --json combined with --format quiet gives the wrong output shape. Both reviewers found this. (internal/cli/deliverability/helpers.go:165-169)

This is known follow-up #1, but it is more than a lost next_cursor.

• The package-local isQuiet() reports quiet mode.
• common.GetOutputWriter gives --json precedence, so it writes JSON.
• The result is a bare JSON array of full rows. That is neither the ID-only quiet output nor the {data, next_cursor, request_id} envelope.

Codex reproduced this against a fake API:

send-events list --start 7d --json                 -> envelope + stderr "Next page: --page-token page2 --start …"
send-events list --start 7d --format quiet --json  -> bare [ {...full row...} ], no next_cursor, no page hint
send-events list --start 7d --quiet --json         -> "aaa"

Both event feeds are affected. The root cause is a local helper that disagrees with the shared output-format resolution. CLAUDE.md says to rely on common.GetOutputWriter, not a parallel check.

Fix: decide quiet mode from the effective format the shared writer resolves, and add a regression test for --format quiet --json.

---

Minor

2. The next-page hint loses the window after page 1. (send_events.go:32-61, helpers.go:134-144)

pageFlags is only set when --start or --end is a look-back.

• Case A: list --start 7d prints Next page: --page-token T --start E. Page 2 is run with those exact flags, so its --start is absolute, and it prints only Next page token: T2. The --start E reminder is gone from page 3 onward.
• Case B: list --end 2026-10-01 makes the CLI compute and send a start the user never typed (6 days before --end). The hint is still just Next page token: T. If the user then runs --page-token T without --end, the token no longer matches the query.

Fix: whenever a cursor comes back, print the start and end values that were actually sent, not only after a look-back.

3. A --start later than --end is sent to the API. (send_events.go:32-61)

resolve() never compares the two values. --start 7d --end 2026-10-01 sent start 2026-10-26 and end 2026-10-01.

Fix: reject it locally, the same way the 90-day --end check works.

4. --format yaml output has a different shape from --json and the API. (internal/domain/deliverability_*.go)

• The new types have no yaml tags, so yaml.v3 lowercases the Go field names: nextcursor, eventtype, bouncedhard.
• The embedded structs make it worse: yaml.v3 nests them, while JSON flattens them. They are:
  • SendMetricsBucket{SendCounts} and GradedRateTile{RateTile} (deliverability_metrics.go:47-51, 100-107)
  • TrackingMetricsBucket and TrackingMetricsRow{EngagementCounts} (159-181)
  • TrackingClickTotals{TrackingTotals} (deliverability_activity.go:175-178)

Missing yaml tags are common across the codebase. But docs/commands/deliverability.md presents YAML as a usable output for paging, so it matters more here.

Fix: add yaml:",inline" to the embedded fields, and add yaml tags that match the JSON names.

5. No test pins "POST is not retried". (adapters/nylas/deliverability_alerts.go:36-38)

The PR makes this guarantee, but if doJSONRequestNoRetry were changed to doJSONRequest, every test would still pass.

Fix: add a test where httptest returns 503, then assert exactly one request and an error.

6. Rule IDs and message IDs are not validated, so .. reaches the server.

Locations: deliverability_alerts.go:77-79, deliverability_activity.go:57,107.

url.PathEscape("..") returns .. unchanged, so send-alerts delete .. --yes sends DELETE /v3/alerts/send/... A gateway that normalizes paths would turn that into DELETE /v3/alerts.

The risk is low: it is the user's own key and argument, and other commands have the same pattern. But this PR already validates event IDs, so rule IDs are the odd one out.

Fix: reject . and .., or check rule IDs for UUID shape.

---

Nits

• Weak test assertion. In TestSendSummary_NullRatesAreNotZero, the check assert.Contains(out, "—") always passes because of the Prior period: — line. Only the NotContains "0.00%" check guards anything.
• --end is not always inclusive. --end 2026-10-01T23:59 resolves to 23:59:00, but a date-only --end resolves to 23:59:59. The help text says "inclusive".
• Breakdown --limit ignores bad values. --limit 0 or a negative value is dropped (AddInt only sends values above 0), so the server default of 25 applies. Rejecting it would be clearer.
• Docs:
  • deliverability.md:115 says "--quiet prints one ID per line", but message, the metrics commands and delete print nothing in quiet mode.
  • The same line calls the JSON "the full API response". It is re-encoded through the typed structs, so unknown fields, subject: null and an empty next_cursor are dropped.
  • deliverability.md:81 leaves --order out of the tracking filter list.
• Demo client: ListSendEvents and ListTrackingEvents ignore all filters. That is fine for a demo, but worth a comment.

---

Checked and correct

• Time parsing:
  • Values are UTC, and a date-only --end runs through 23:59:59.
  • Invalid dates (2026-02-30), millisecond timestamps, today and 9am, and dates before 1970 are rejected.
  • Huge look-backs (200000d), 0 and -7d are refused instead of silently becoming the server default.
  • Mixed case and surrounding whitespace on look-backs work, and there is no DST exposure.
• Window rules:
  • The 90-day --end check is correct at the day edge.
  • When only --end is set, the default start is truncated to midnight.
  • A look-back combined with --page-token is refused before any API call.
• Query encoding: event_type is sent as repeated keys, unset filters are omitted, and metrics-only parameters don't leak between routes. Message IDs with <>/?# are escaped.
• Send-alert update (PUT): it sends only the flags the user set, an explicit --min-volume 0 is still sent, and an empty update is refused. --threshold goes out as a JSON number with the user's exact decimals.
• Retries: POST is not retried. PUT is retried, which is safe because PUT is idempotent.
• Null rates: they stay distinct from 0 and print as —.
• Conventions:
  • delete requires --yes, matching the other commands.
  • Success messages are suppressed in structured output modes.
  • The mock, the demo client and HTTPClient all satisfy the port.
  • It uses WithClientNoGrant, AddLimitFlag, AddPageTokenFlag and NewQueryBuilder, with no custom --format flag.
• Integration tests: they call acquireRateLimit, and the alert rule they create (threshold 99.5, min-volume 1,000,000) never fires and is cleaned up.
• Security: no secrets appear in errors or test fixtures.

Nice work overall. The page-token-to-window binding and the scope statement in every command's help are well thought out.

🤖 Generated with Claude Code (Claude and Codex reviews)

[jess-nylas]

Thanks for the thorough review. Fixed in 7c562a5:

• 1. --json --format quiet shape: isQuiet now uses common.GetOutputOptions(cmd, nil).Format (--quiet, then --json, then --format), so it always agrees with the writer. --format quiet --json now prints the {data, next_cursor, request_id} envelope, and --quiet --json still prints IDs. Regression test: TestSendEventsList_JSONBeatsFormatQuiet.
• 4. YAML shape: every deliverability output type has yaml tags that mirror its json tags. The embedded SendCounts, RateTile, EngagementCounts and TrackingTotals fields are yaml:",inline". TestDeliverabilityYAMLMatchesJSON round-trips a populated fixture of each output type through YAML and compares it with the JSON. On staging, --format yaml matches --json for list, summary, timeseries, breakdown, tracking message and alerts list.

The remaining items (2, 3, 5, 6 and the nits) are tracked separately; I'll follow up on them.

[qasim-nylas]

LGTM 👍