Skip to content

Add opt-in self-host collector analytics - #636

Merged
kjgbot merged 5 commits into
mainfrom
agent/telemetry-file-sink
Nov 11, 2026
Merged

kjgbot merged 5 commits into
mainfrom
agent/telemetry-file-sink

Conversation

@khaliqgant

@khaliqgant khaliqgant commented Nov 8, 2026 •

Copy link
Copy Markdown
Member

Adds a default-off self-host analytics producer for Relaycast. A typed projector and bounded asynchronous batcher send a closed vocabulary to the installation collector. Existing hosted telemetry adapters keep their current configuration.

The reviewed privacy repairs bind native workspace attribution and keep exception classifications closed. The updated head reconciles the branch with current main and wires the same collector lifecycle through the SQLite and PostgreSQL entrypoints. Shutdown settles abandoned batches before reporting completion, and a queue consisting entirely of expired events releases its pump.

Exact head 25eee4e0 passed the hosted container, lint, build, engine, and SDK checks. A clean worktree install, the full Node engine suite (2,945 passed), and 47 container entrypoint tests also passed. The built arm64 image delivered an authenticated direct message and exported its typed event to a trusted HTTPS collector. With an untrusted certificate, messaging still worked and the collector received no event. The matching collector vocabulary round-tripped all 60 kinds in 136 producer envelopes.

This PR ships source support with the default off. A deployment must select an image that includes the producer and provision a collector registration with a dedicated credential, matching installation ID, and CA.

Relaycast Agent Host and others added 2 commits November 8, 2026 01:27
Add CollectorTelemetrySink, an opt-in TelemetrySink that projects engine
observations and exceptions through a typed allowlist into a versioned
export envelope and POSTs bounded batches to an installation-local
collector Service using the agent-relay.analytics.batch/v1 wire contract
(infra PR 84). The default no-op sink and the telemetry port are
unchanged; nothing is exported unless the collector URL is configured,
and there is no hosted fallback.

- telemetry/exportEnvelope.ts: pure projection; required facts per kind
  plus a small per-kind optional allowlist; actor type only, never the
  actor id, credential hash, sender, content, tokens or request data;
  exceptions export only error class name, code-shaped code, allowlisted
  operation and workspace id.
- telemetry/batcher.ts: bounded queue, one batch in flight, bounded
  retries with backoff, split and terminal hints, shutdown drain with
  deadline, counters (enqueued, exported, dropped, queueFull, rejected,
  failedBatches, retries, splits), rate-limited stderr line, never
  re-captured into a sink.
- telemetry/httpTransport.ts: the only wire module; schema id, field
  mapping, bearer credential, response table (400/401/403/other 4xx and
  redirects terminal, 413 split, 429/503/507/5xx/timeouts retried with a
  capped Retry-After), https-only except loopback, *.svc and
  single-label hosts, redirects never followed, bodies never read.
- Node adapter and startServer accept telemetryCollector; stop() drains.
- docker entrypoint: RELAYCAST_TELEMETRY_COLLECTOR_URL, _TOKEN(_FILE),
  _DEPLOYMENT, _QUEUE_MAX, _BATCH_MAX, _FLUSH_MS; malformed values and a
  pinned engine without collector support exit 64 before engine import.
- Tests for projection, batcher, transport, sink, entrypoint parsing and
  a Node wiring integration test against a local HTTP collector.
- docs/self-hosting.md and changelogs.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…cations

- Only 204 is acceptance; any other 2xx is a retryable protocol error.
- The collector URL is the complete endpoint (no path appended); a query
  string or fragment is refused, in the transport and the entrypoint.
- Retry-After: unsigned decimal delta-seconds only, capped at 30 s, clipped
  to the delivery and shutdown deadlines; HTTP-dates and malformed values
  are ignored and 0 never allows extra attempts.
- Per-event total delivery deadline (deliveryDeadlineMs, default 60 s) from
  enqueue across queued time, attempts and waits; splits and retries keep
  it, per-attempt timeouts are clipped to it, exhaustion counts `expired`.
- error_name / error_code come from registered allowlists ('unregistered'
  or omitted otherwise); tenant_id is never sent.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 6 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 7a773b91-5bc5-492b-bc5d-4436b26517f2
Commits

Reviewing files that changed from the base of the PR and between 34c5dce and 25eee4e.

Files selected for processing: 19

  • CHANGELOG.md
  • docker/entrypoint-core.mjs
  • docs/self-hosting.md
  • packages/engine/CHANGELOG.md
  • packages/engine/src/adapters/node/__tests__/telemetry-collector.test.ts
  • packages/engine/src/adapters/node/index.ts
  • packages/engine/src/entrypoints/node.ts
  • packages/engine/src/index.ts
  • packages/engine/src/lib/serverTelemetry.ts
  • packages/engine/src/ports/telemetry.ts
  • packages/engine/src/providers/__tests__/collector-telemetry.test.ts
  • packages/engine/src/providers/collector-telemetry.ts
  • packages/engine/src/telemetry/__tests__/batcher.test.ts
  • packages/engine/src/telemetry/__tests__/exportEnvelope.test.ts
  • packages/engine/src/telemetry/__tests__/httpTransport.test.ts
  • packages/engine/src/telemetry/batcher.ts
  • packages/engine/src/telemetry/exportEnvelope.ts
  • packages/engine/src/telemetry/httpTransport.ts
  • test/container-entrypoint.test.mjs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Privacy: replace the regex-shaped fact filter with a telemetry-owned
analytics allowlist. ANALYTICS_FIELD_RULES gives every exported field a
value rule (opaque id, count, number, flag or fixed enum -> 'other');
ANALYTICS_EVENT_FIELDS lists each kind's fields; ANALYTICS_DROPPED_FIELDS
names the port fields never exported (agent_name, invited_agent_name,
channel_name, command, action_name, slug, skill, emoji, provider). Origin
strings become registered enums (origin_app, origin_actor_type,
origin_client); the actor name and client version are gone, as is the
hostname `instance`. Tests enumerate every ENGINE_OBSERVATION_FACTS field.

Classification: error names and codes are exported only from the
registries; code-shaped strings are no longer a path.

Delivery: only 204 is acceptance (200/202/206 retried in budget, then
lost). 401/403 are terminal configuration failures (configFailures
counter, CONFIGURATION FAILURE log). Redirects are manual and terminal.
413 splits to a bounded depth and counts oversized events. Per-event,
per-batch and queue budgets are measured in UTF-8 bytes; oversized
events are dropped and counted; queue exhaustion drops the newest event
and counts queueFull. trace_id is always set (request id, else a random
UUID) and stays stable across retries and splits.

Config: when the collector is enabled, a bearer token and an explicit
deployment id are required (no hostname fallback), refused with
missing_telemetry_collector_token / missing_telemetry_collector_deployment_id
in the engine and the container entrypoint. All bounds are configurable
through options and RELAYCAST_TELEMETRY_* env with conservative producer
defaults (infra#85 publishes no numbers), documented in
docs/self-hosting.md and the changelogs.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copy link
Copy Markdown
Member Author

The claim that 1c3f583c fully matches the merged collector contract is not accepted by this review. Queue counts, per-request timeout, attempt count, UUID stability and redirect refusal are useful but do not resolve the reproduced findings recorded in this PR.

At that exact source head, the synthetic source-level reproducer confirmed: agent_name product facts are exported; arbitrary exception code and constructor-name strings are exported; HTTP 200 is accepted as success; required trace_id may be missing; collector mode can be enabled without the dedicated producer token; and deployment identity defaults to replica hostname. These are seven observed violations, not hypothetical requests. Independently, static review found no configurable event/batch/queue byte limits or cumulative event delivery deadline from enqueue; a 5-second per-request timeout and three attempts do not implement those requirements.

Please retain #636 as HOLD and queue repairs with causal tests on the same source branch. PostgreSQL implementation may remain the first priority; this comment does not request a priority switch. The canonical contract is AgentWorkforce/infra/docs/analytics-collector-interface.md, including merged #85 clarifications. Workspace-only Relaycast identity is allowed there; do not invent a tenant ID. Send a corrected exact SHA and test receipts when ready. No producer enablement, release or container compatibility is inferred from the current source.

@agent-relay-code

Copy link
Copy Markdown
Contributor

Corrected exact SHA for the repairs: 48a2086649b537f8cfc24de1ef393fbd9736f4d7 (agent/telemetry-file-sink, current PR head, pushed 2026-11-08T02:59Z). The reproducer findings above were taken at 1c3f583c; 48a20866 sits two commits later on the same branch. Mapping of each recorded finding to the fix, with its causal test:

  1. agent_name product facts exported → exportEnvelope.ts now owns its own analytics allowlist (ANALYTICS_FIELD_RULES / ANALYTICS_EVENT_FIELDS / ANALYTICS_DROPPED_FIELDS) and no longer reads ENGINE_OBSERVATION_FACTS. agent_name, invited_agent_name, channel_name, command, action_name, slug, skill, emoji, provider are dropped; ids must be snowflake/UUID/registered-prefix shaped; enums outside the registered set become other. Test: every ENGINE_OBSERVATION_FACTS field is asserted as either ruled or dropped, plus agent.updated agent_name regression.
  2. Arbitrary exception code / constructor-name strings exported → only REGISTERED_ERROR_NAMES / REGISTERED_ERROR_CODES leave; unregistered values that match the old regexes are tested as not exported.
  3. HTTP 200 accepted as success → 204 is the only acceptance; 200/202/206 are protocol failures (tested), retried within the finite budget then counted as loss. Redirects use redirect: 'manual', are never followed or re-sent with the token (301/302/303/307/308 tested). 401/403 terminal with configFailures + CONFIGURATION FAILURE log; 400 terminal rejected; 413 bounded split; 507 bounded retry.
  4. trace_id may be missing → every event carries a non-empty trace_id: the well-formed request_id, else a UUID generated once at projection, stable across retries and 413 splits (tested).
  5. Collector mode enabled without producer token → CollectorConfigError('missing_telemetry_collector_token'); container entrypoint exits 64 with the same code.
  6. Deployment identity defaulted to replica hostname → explicit RELAYCAST_TELEMETRY_DEPLOYMENT_ID required (pattern-validated, missing_/invalid_telemetry_collector_deployment_id); hostname default and ExportIdentity.instance removed; old RELAYCAST_TELEMETRY_DEPLOYMENT deliberately not read (tested).
  7. Byte limits / cumulative deadline → eventMaxBytes 8192 / batchMaxBytes 262144 / queueMaxBytes 1048576 measured on the exact UTF-8 wire encoding, configurable via options and RELAYCAST_TELEMETRY_{EVENT,BATCH,QUEUE}_MAX_BYTES; queue exhaustion drops newest and counts queueFull; oversized events dropped and counted; cumulative deliveryDeadlineMs from enqueue covering queue time, attempts and waits, never reset by split/retry, clipped by shutdown (tested, multi-byte strings included).

tenant_id is still never emitted. Receipts at 48a20866: tsc 0, eslint 0, telemetry/providers/node-collector vitest 171/171, container entrypoint 39/39, full engine suite 178 files / 2137 tests passed. No producer enablement, release or container compatibility is claimed; docker still pins engine 12.8.0, so the container refuses collector config with unsupported_telemetry_collector until a release bump.

Copy link
Copy Markdown
Member Author

Review of exact fix head 48a2086649b537f8cfc24de1ef393fbd9736f4d7: the seven original synthetic repros now behave correctly (names and unregistered exception class/code omitted; HTTP 200 refused; trace present; token and explicit deployment identity required). The separate typed analytics registry, byte bounds and deadline inheritance are present. Two residual P2 findings keep source clearance held.

  1. Validate workspace attribution values, including exception context. exportEnvelope.ts:431-432 still accepts any nonempty string up to 128 characters as workspace identity. A synthetic email-shaped marker supplied as observation workspaceId or exception workspace_id is exported unchanged. This bypasses the new opaque-ID rules applied to facts and the stated no-email/no-product-text projection. Use the reviewed native engine workspace-ID grammar: reject/drop invalid workspace observations; omit or explicitly classify invalid exception attribution without forwarding raw text. Include valid actual engine workspace-ID controls, not just arbitrary test strings.

  2. Shutdown does not cancel an attempt that started before close. BatchAttempt supplies only a timeout snapshot, and batcher.shutdown() has no abort path to the HTTP controller. I reproduced a collector send already in flight with a 1000ms request timeout, then called close() with a 20ms shutdown budget. Close resolved while the supplied fetch signal remained not aborted. At that point counters were enqueued=1/exported=0/dropped=0/queued=0; releasing a synthetic late 204 then changed exported to 1 after close. The existing timeout test only starts a new attempt after shutdown begins. Carry cancellation to the active transport, abort it at shutdown deadline, settle/count ambiguous delivery once, and ignore a late completion. Test both an already-running request and queued/retry/split work; preserve the no-exactly-once claim.

Validation at this exact head: Bun 1.4.2, one Vitest worker, no file parallelism, all through aw-heavy. Vitest reported 5 files / 171 tests passed, but the queue wrapper then exited 1 with a cleanup PermissionError; this is not a clean outer-command success. The independent Node 24.22.2 container entrypoint run was 39/39 pass, exit 0. The separate synthetic mock-transport repro exited 0 and demonstrated both residual findings. Queue is idle. No shared fixture, real credential, hosted export, deployment or publication used. I have not rerun the full engine suite, typecheck or lint.

Container release gap remains separate: pinned engine 12.8.0 still refuses collector mode; no compatible released image or retained-file end-to-end proof follows from this source review. Please return the corrected frozen head and causal tests.

Repairs the #636 review (HOLD) and re-review findings against the
agent-relay.analytics.batch/v1 collector contract, each with a causal test
(named F1..F10) that fails on 48a2086 and passes here.

- F1/F2: properties are re-projected at the wire through the per-kind
  allowlist and the fixed exception classification (exportProperties).
- F3: only 204 is acceptance; other 2xx are counted protocol errors.
- F4: trace_id is mandatory and never invented; no tenant_id; fixed scope
  registry (EXPORT_EVENT_SCOPES) enforced by validateExportEvent in the
  sink and the transport.
- F5: RELAYCAST_TELEMETRY_DESTINATION / telemetryDestination; collector
  mode without URL or token is a hard error; token charset is validated
  without echoing it.
- F6: a deployment id equal to a replica hostname is refused.
- F7/F8: in-flight batches count against queue bounds; split halves
  inherit the parent deadline; attempts are abandoned at the deadline even
  if the transport ignores timeoutMs; BACKOFF_MS and RETRY_AFTER_MAX_MS
  knobs (Retry-After capped at 30 s).
- F9: workspace_id must match the native workspace-id grammar
  (isArWorkspaceId); other text is counted invalid and never exported.
- F10: at the shutdown deadline the in-flight attempt is aborted through
  BatchAttempt.signal, counted once, and a late completion is ignored.
  Delivery stays at-least-once at best; no exactly-once guarantee.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@agent-relay-code

Copy link
Copy Markdown
Contributor

Fix head for review 6717272197: f6da7b345d45b2e0d07ee0c2da56a7eaa1e84609 on agent/telemetry-file-sink (one commit on top of 48a20866, history not rewritten, pushed). PR stays draft / HOLD; nothing enabled in the chart, nothing published, no analytics acceptance claimed.

Residual 1 (workspace attribution grammar) exportEnvelope.ts workspaceIdOf now accepts only the engine's native workspace-id grammar via isArWorkspaceId from @relaycast/types (the check ar_ credentials use; what generateWorkspaceId mints and deriveKeyedWorkspaceId derives). Observation path: an observation whose workspaceId fails the grammar projects to null, the sink counts it as invalid, nothing is exported. Exception path: exception is registered installation-only in EXPORT_EVENT_SCOPES, so it never carries workspace_id at all (always null), whatever the context holds; no raw text is forwarded. validateExportEvent is applied at the sink gate and again in the transport as a backstop. Design note for your call: I chose omission over classification for exception attribution, so a valid workspace id on an exception is also dropped.
Tests (exportEnvelope.test.ts, collector-telemetry.test.ts): controls with ids minted by generateWorkspaceId and derived by deriveKeyedWorkspaceId still export; 9 observation-path and 9 exception-path negatives (email-shaped, product-looking text, ws_1, over-long digits/text, leading and inner whitespace, above 64 bits, leading zero); "the exception path never carries even a valid workspace id"; sink counts a non-native id as invalid and never sends it.

Residual 2 (shutdown cancels an in-flight attempt) batcher.ts attemptSend: one AbortController per attempt, passed to the transport as BatchAttempt.signal; httpTransport.ts wires it to the fetch controller and reports collector request cancelled. At the shutdown deadline the batcher calls cancel() on the active attempt: the signal aborts, the batch is counted dropped exactly once ("the collector may already have committed it"), and the attempt is marked settled. exported is now incremented in the same step that settles the attempt, so a late 204 (or late failure) after settlement changes no counter. Split halves inherit the parent deadline; queued events, a batch in retry wait and a pending second half are dropped and counted once at the deadline with no new request started. The attempt timeout aborts the same signal even if the transport ignores timeoutMs. Docs keep the at-least-once / never exactly-once wording.
Tests (batcher.test.ts, httpTransport.test.ts): already-running request at close → signal aborted, close resolves, counters settled, synthetic late 204 ignored; late failure ignored; queued events dropped once; batch in retry wait dropped with no new request; split in flight; attempt timeout aborts the signal; transport abort mid-request; signal already aborted. The existing "attempt started after shutdown" test still passes.

Also in this head (repairs from the first review that were only partly covered at 48a2086, each with a causal test): F1 per-kind allowlist re-applied in toWireEvent via exportProperties; F2 fixed classifications re-checked at the wire; F3 200/202/206 counted as protocolErrors, 507 as storageFailures, redirects as redirects; F4 trace_id is the engine request id, never invented (missing → invalid, counted); F5 explicit RELAYCAST_TELEMETRY_DESTINATION (none | collector, anything else refused, no PostHog fallback), token must be visible ASCII, never echoed; F6 a deployment id equal to the replica hostname / $HOSTNAME is refused; F7 in-flight batch counts against queueMax / queueMaxBytes; F8 retryAfterMaxMs (≤ 30 s, knob RELAYCAST_TELEMETRY_RETRY_AFTER_MAX_MS), RELAYCAST_TELEMETRY_BACKOFF_MS.

Causality check 48a2086 source files restored under the new tests: 124 of 229 vitest tests fail and 10 of 47 container tests fail; the only F-tagged tests that already held on 48a2086 are the 129-char exception negative and the single-Authorization-header guard, which is now named as a guard, not a causal test.

Receipts at f6da7b34 (Node, ~/work/telemetry-file-sink)

  • packages/engine: tsc --noEmit exit 0; npm run lint 16/16 tasks.
  • Focused vitest (telemetry, providers, node adapter collector test): 5 files / 229 tests passed; the same with --no-file-parallelism --maxWorkers=1: 5 files / 229 passed, exit 0.
  • Full engine vitest: 178 files / 2195 tests passed, exit 0.
  • npm run test:container: 47/47 pass, exit 0.
  • Not done: Bun run, aw-heavy queue wrapper. Container release gap (pinned engine 12.8.0 refuses collector mode) remains separate and unaddressed here.

🤖 Generated with Claude Code

@kjgbot

kjgbot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Selected delta review at f6da7b3, against the previous reviewed head 48a2086: the two P2 findings in 6717272197 are resolved at this head.

  • Workspace attribution now uses the native isArWorkspaceId grammar; malformed observation attribution is refused, while exception attribution is omitted even for a valid workspace. A separate synthetic reproduction verified a canonical workspace positive, four invalid workspace controls, omitted exception attribution and omission of a planted product name from the wire.
  • An HTTP attempt already running before close() receives an aborted signal at the shutdown deadline. The ambiguous batch is counted dropped once, and a late synthetic 204 does not change the settled export/drop counters. Retry and split work retain their deadline and cancellation bounds.

New P3: immediate shutdown accounting is not fully settled. A real-clock reproduction through CollectorTelemetrySink (not fake timers) sees await sink.close() return with inFlight=1, inFlightBytes=376, despite exported=0, dropped=1 and the request signal already being aborted. After one event-loop turn, before releasing the synthetic late 204, these become zero. The late response then changes no counters. batcher.ts resolves shutdown() at its deadline before pump() reaches its finally cleanup. The fake-timer tests drain those microtasks before inspecting the result, masking this snapshot gap. Retire the held counters before fulfilling close (without double-accounting split halves), or wait for the already-cancelled pump cleanup; add a real-clock positive control. This is a transient metrics issue, separate from the original uncancelled-request/late-export P2.

Independent validation on this exact head: real npm ci, Node 24.22.2, own workspace dependencies; 227 telemetry/provider tests plus 2 Node-adapter tests (229 total), engine typecheck, build, touched-path lint, and 47 container-entrypoint tests all pass. Checks used one Vitest worker and no file parallelism. No independent full-engine-suite rerun is claimed.

This is selected source evidence, not landing or runtime clearance: the PR is conflicting against its current base, the pinned released engine/container still refuses collector mode, and the deployed collector has no Relaycast event registry yet. Compose onto current main, obtain exact-head CI, coordinate the typed collector registry, then qualify the compatible package/image before enabling a producer. No release, credentials, chart enablement or live export was performed by this review.

Keep the single collector lifecycle across the SQLite and PostgreSQL entrypoints. Settle outstanding queue accounting before shutdown resolves, and clear a pump that expires every queued event synchronously.

Validated with the real worktree npm ci, all three workspace builds, 280 focused tests, and 47 container-entrypoint tests on Node 24.22.2. Relative to main the diff passes git diff --check.
@kjgbot kjgbot changed the title Add opt-in collector analytics producer (privacy and contract fixes pending) Add opt-in self-host collector analytics Nov 11, 2026
@kjgbot
kjgbot marked this pull request as ready for review November 11, 2026 00:49
@kjgbot
kjgbot merged commit cac175b into main Nov 11, 2026
18 checks passed

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 potential issues.

Devin Review

*/
const SNOWFLAKE_ID = /^[0-9]{1,20}$/;
const UUID_ID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
const PREFIXED_ID = /^(?:dm|dmch|gdm|sub|wh|whd|inv|act|file|aud|cmd|node)_[A-Za-z0-9-]{8,96}$/;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Identity events lose their audit IDs

When an agent identity changes, isOpaqueId rejects its aid_ audit ID. Exported takeover and revocation events omit the audit record identifier.

Learn more

The engine generates identity audit records with aid_ IDs in agentIdentity. The takeover and revocation routes pass those IDs as audit_id to emitServerEvent. The analytics field rule for audit_id calls isOpaqueId, which rejects every generated aid_ ID. Both events therefore export without their audit IDs.

Example: An identity takeover creates aid_1189827393826947072. Its exported agent.identity.taken.over event retains agent_id but has no audit_id, preventing correlation with that audit record.

Recommended fix: Add the aid prefix to isOpaqueId and test both identity event projections using an aid_ ID generated by the engine.

Suggested change
const PREFIXED_ID = /^(?:dm|dmch|gdm|sub|wh|whd|inv|act|file|aud|cmd|node)_[A-Za-z0-9-]{8,96}$/;
const PREFIXED_ID = /^(?:dm|dmch|gdm|sub|wh|whd|inv|act|file|aud|aid|cmd|node)_[A-Za-z0-9-]{8,96}$/;

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

*/
export function projectException(err: unknown, context: unknown, identity: ExportIdentity): ExportEvent | null {
const ctx: Record<string, unknown> = context && typeof context === 'object' ? context as Record<string, unknown> : {};
const traceId = traceIdOf(ctx.request_id ?? ctx.requestId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Exception context gains forbidden casing fallback

When request_id is absent, projectException accepts requestId as its trace. This mixed-case field fallback violates the repository's explicit casing rule.

Suggested change
const traceId = traceIdOf(ctx.request_id ?? ctx.requestId);
const traceId = traceIdOf(ctx.request_id);

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

- Collector delivery: queue count, queue bytes, batch count, batch bytes, event bytes, flush interval, attempts, request timeout, delivery deadline and shutdown timeout are all bounded and configurable (`queueMax` 1000, `queueMaxBytes` 1048576, `batchMax` 50, `batchMaxBytes` 262144, `eventMaxBytes` 8192, `flushIntervalMs` 2000, `maxAttempts` 3, `requestTimeoutMs` 5000, `deliveryDeadlineMs` 60000, `shutdownTimeoutMs` 5000). These are conservative producer defaults; the collector contract does not publish numbers. Byte budgets are measured in serialized UTF-8 bytes (`utf8Bytes`, `wireEventBytes`, `WIRE_BATCH_FRAME_BYTES`): an event over `eventMaxBytes` is dropped and counted as `oversized`, batches are formed under both `batchMax` and `batchMaxBytes`, and when the queue count or byte budget is exhausted the newest event is dropped and counted as `queueFull`. The per-event delivery deadline starts at enqueue and covers queued time, all attempts and all waits; a batch uses its earliest event's deadline, retries and `413` splits never reset it, each request timeout is clipped to the time left, and exhausted events are dropped and counted as `expired`. Only `204` is acceptance; `200`, `202`, `206` and any other `2xx` are protocol errors, retried within the budget and then counted as loss. Collector responses follow the collector contract: `400` and other `4xx` are terminal and counted as `rejected`; `401`/`403` are terminal configuration failures (counted in `configFailures` and logged with a `CONFIGURATION FAILURE` prefix under their own rate limit); redirects use `redirect: 'manual'` and are never followed or retried with the token; `413` splits the batch to a bounded depth and a single event that is still too large is counted as `oversized`; `429`/`503`/`507`/other `5xx`/timeouts are retried. `Retry-After` is honored only as unsigned decimal delta-seconds, capped at 30 s and clipped to the delivery and shutdown deadlines (HTTP-dates and malformed values are ignored; `0` never allows extra attempts). `stats()` reports `enqueued`, `exported`, `dropped`, `queueFull`, `oversized`, `configFailures`, `rejected`, `expired`, `failedBatches`, `retries`, `splits` and `queuedBytes`. Failures log a rate-limited stderr line and are never re-captured. The URL is the complete endpoint including `/v1/events` (no path is appended) and must not carry credentials, a query string or a fragment; it must be https except for loopback, `*.svc` / `*.svc.cluster.local` and single-label hosts.
- `createNodeRuntime` / `startServer` accept `telemetryCollector: { url, bearerToken, deployment, queueMax?, queueMaxBytes?, batchMax?, batchMaxBytes?, eventMaxBytes?, flushIntervalMs?, maxAttempts?, requestTimeoutMs?, deliveryDeadlineMs?, shutdownTimeoutMs? }` (an explicit `telemetry` still wins). `bearerToken` and `deployment` are required, with no hostname fallback: a missing one throws `CollectorConfigError` with code `missing_telemetry_collector_token` or `missing_telemetry_collector_deployment_id`, and a malformed deployment id throws `invalid_telemetry_collector_deployment_id`. `NodeRuntime.closeTelemetry()` drains the sink and `RunningServer.stop()` awaits that drain. Without the option the no-op sink is unchanged.

- Collector review repairs (#636): `exportProperties(event)` re-projects properties at the wire through the per-kind allowlist and the fixed exception classification, so a hand-built `ExportEvent` cannot export `agent_name`, other product facts, or raw exception names/codes. `EXPORT_EVENT_SCOPES` fixes each kind's scope (observations: workspace; `exception`: installation-only, `workspace_id` always null) and `validateExportEvent(event, identity?)` returns an `ExportEventInvalidReason` (`unregistered_kind`, `missing_trace_id`, `missing_event_id`, `missing_workspace_id`, `unauthorized_workspace_id`, `tenant_id_not_allowed`, `deployment_mismatch`, `component_mismatch`); the sink counts refused events in `stats().invalid` (`CollectorTelemetryStats`) and the HTTP transport refuses such a batch before sending. Non-`204` `2xx`, `507` and redirects are counted in `protocolErrors`, `storageFailures` and `redirects`. The bearer token must match `BEARER_TOKEN_PATTERN` (visible ASCII, no whitespace), else `invalid_telemetry_collector_token`; a deployment id equal to the replica hostname (`replicaHostnames`, by default `os.hostname()` and `HOSTNAME`) is refused with `invalid_telemetry_collector_deployment_id`. `resolveTelemetryDestination` / `TELEMETRY_DESTINATIONS` and `telemetryDestination` on `createNodeRuntime` / `startServer` select `none` or `collector` explicitly (`missing_telemetry_collector_config`, `unsupported_telemetry_destination`). New options `backoffMs` (500) and `retryAfterMaxMs` (30000, at most 30000). The batch being sent counts against `queueMax` / `queueMaxBytes` (`inFlight`, `inFlightBytes`); `413` split halves inherit the parent's delivery deadline; an attempt that outlives its timeout is abandoned even if the transport ignores `timeoutMs` (`attemptTimeouts`). `workspace_id` must match the native workspace-id grammar (`isArWorkspaceId`, exported as `workspaceIdOf`); observations with any other workspace text are counted `invalid` and never sent, and exceptions never carry a workspace id. At the shutdown deadline the batcher aborts the in-flight attempt through the new `BatchAttempt.signal` (the HTTP transport cancels its fetch), counts that batch once as dropped and ignores a late completion; no exactly-once delivery is implied.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Collector changelog publishes review backstory

The Collector review repairs (#636) entry exposes internal review notes in release notes. The changelog rules require concise, impact-first entries without PR references or implementation backstory.

Learn more

The package changelog is published as user-facing release guidance. Its new collector entries span several long implementation lists, and the review-repairs bullet includes a PR number and internal review narrative. The repository's changelog rules require one short, impact-first bullet per user-visible change and exclude internal review notes and PR references.

Example: A release reader encounters Collector review repairs (#636) and detailed notes about internal fixes instead of a short description of what opting into collector analytics provides.

Recommended fix: Consolidate the new collector bullets into concise user-visible release notes; remove the review-repairs bullet and PR reference, preserving API details only where they help users migrate or configure the feature.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

@greptile-apps

greptile-apps Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The PR adds an opt-in analytics path across self-host server startup and engine observations.

The repository’s field-naming and changelog requirements must be satisfied before merging; the image-support warning also needs correction.

Fix All in CodexFindings

  1. P2 Two request-id spellings ▶
  2. P2 Release notes bury the change ▶
  3. P2 Image support is described incorrectly ▶
Fix with agent prompt
### Issue 1
packages/engine/src/telemetry/exportEnvelope.ts:545
`projectException` adds `ctx.request_id ?? ctx.requestId`, which violates the repository directive “Do not introduce mixed-case field fallbacks.” Accept only `request_id` and remove the test that requires the alternate spelling. This repository requirement must be satisfied before merging.

### Issue 2
packages/engine/CHANGELOG.md:24
The new entries repeat the configuration and delivery specification, and this entry includes internal review history. `AGENTS.md` requires one short, impact-first bullet per user-visible change and says to omit internal review notes. Summarize the shipped behavior and link to the detailed guide before merging. The same pattern appears in `CHANGELOG.md:27` and `packages/engine/CHANGELOG.md:21`, among other new entries.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

### Issue 3
docs/self-hosting.md:161-164
This paragraph says collector support requires updating the pinned npm release, but `Dockerfile` builds the checked-out engine and copies its `dist` over the installed package. An image built from this branch therefore includes collector support without changing that pin. Operators may wait for an unnecessary update or expect a startup refusal that will not happen. Describe support in terms of the selected image, and reserve `unsupported_telemetry_collector` for an engine that actually lacks the export.

```suggestion
The container image builds the engine from its checkout and replaces the
installed package's `dist`, so an image built from this source includes collector
support without changing the npm pin. Select an image that includes this support.
If its bundled engine lacks the collector export, setting
`RELAYCAST_TELEMETRY_COLLECTOR_URL` stops startup with exit code `64` and
`unsupported_telemetry_collector` rather than silently ignoring the setting.
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

Adds default-off analytics export to an installation-owned collector.

  • Self-host analytics keeps only approved event details.
  • Collector events travel in bounded batches with retries.
  • Operators can enable collector export through either Node server entrypoint.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Engine observations and exceptions] --> B[Keep approved fields and values]
  B --> C[Bounded queue]
  C --> D[HTTP batch]
  D --> E[Installation collector]
  E -->|204| F[Count accepted events]
  E -->|Temporary failure| G[Retry within deadline]
  G --> D
  E -->|413| H[Split batch]
  H --> D
  C -->|Shutdown| I[Drain or abort at deadline]
Loading

Reviews (1) · Last reviewed commit: "Reconcile Relaycast collector with curre..." · Reviewed by Greptile

*/
export function projectException(err: unknown, context: unknown, identity: ExportIdentity): ExportEvent | null {
const ctx: Record<string, unknown> = context && typeof context === 'object' ? context as Record<string, unknown> : {};
const traceId = traceIdOf(ctx.request_id ?? ctx.requestId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Two request-id spellings

projectException adds ctx.request_id ?? ctx.requestId, which violates the repository directive “Do not introduce mixed-case field fallbacks.” Accept only request_id and remove the test that requires the alternate spelling. This repository requirement must be satisfied before merging.

Context Used: CLAUDE.md (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/engine/src/telemetry/exportEnvelope.ts
Line: 545

Comment:
**Two request-id spellings**

`projectException` adds `ctx.request_id ?? ctx.requestId`, which violates the repository directive “Do not introduce mixed-case field fallbacks.” Accept only `request_id` and remove the test that requires the alternate spelling. This repository requirement must be satisfied before merging.

**Context Used:** CLAUDE.md ([source](https://github.com/agentworkforce/relaycast/blob/main/CLAUDE.md))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Codex Fix in Claude Code

- Collector delivery: queue count, queue bytes, batch count, batch bytes, event bytes, flush interval, attempts, request timeout, delivery deadline and shutdown timeout are all bounded and configurable (`queueMax` 1000, `queueMaxBytes` 1048576, `batchMax` 50, `batchMaxBytes` 262144, `eventMaxBytes` 8192, `flushIntervalMs` 2000, `maxAttempts` 3, `requestTimeoutMs` 5000, `deliveryDeadlineMs` 60000, `shutdownTimeoutMs` 5000). These are conservative producer defaults; the collector contract does not publish numbers. Byte budgets are measured in serialized UTF-8 bytes (`utf8Bytes`, `wireEventBytes`, `WIRE_BATCH_FRAME_BYTES`): an event over `eventMaxBytes` is dropped and counted as `oversized`, batches are formed under both `batchMax` and `batchMaxBytes`, and when the queue count or byte budget is exhausted the newest event is dropped and counted as `queueFull`. The per-event delivery deadline starts at enqueue and covers queued time, all attempts and all waits; a batch uses its earliest event's deadline, retries and `413` splits never reset it, each request timeout is clipped to the time left, and exhausted events are dropped and counted as `expired`. Only `204` is acceptance; `200`, `202`, `206` and any other `2xx` are protocol errors, retried within the budget and then counted as loss. Collector responses follow the collector contract: `400` and other `4xx` are terminal and counted as `rejected`; `401`/`403` are terminal configuration failures (counted in `configFailures` and logged with a `CONFIGURATION FAILURE` prefix under their own rate limit); redirects use `redirect: 'manual'` and are never followed or retried with the token; `413` splits the batch to a bounded depth and a single event that is still too large is counted as `oversized`; `429`/`503`/`507`/other `5xx`/timeouts are retried. `Retry-After` is honored only as unsigned decimal delta-seconds, capped at 30 s and clipped to the delivery and shutdown deadlines (HTTP-dates and malformed values are ignored; `0` never allows extra attempts). `stats()` reports `enqueued`, `exported`, `dropped`, `queueFull`, `oversized`, `configFailures`, `rejected`, `expired`, `failedBatches`, `retries`, `splits` and `queuedBytes`. Failures log a rate-limited stderr line and are never re-captured. The URL is the complete endpoint including `/v1/events` (no path is appended) and must not carry credentials, a query string or a fragment; it must be https except for loopback, `*.svc` / `*.svc.cluster.local` and single-label hosts.
- `createNodeRuntime` / `startServer` accept `telemetryCollector: { url, bearerToken, deployment, queueMax?, queueMaxBytes?, batchMax?, batchMaxBytes?, eventMaxBytes?, flushIntervalMs?, maxAttempts?, requestTimeoutMs?, deliveryDeadlineMs?, shutdownTimeoutMs? }` (an explicit `telemetry` still wins). `bearerToken` and `deployment` are required, with no hostname fallback: a missing one throws `CollectorConfigError` with code `missing_telemetry_collector_token` or `missing_telemetry_collector_deployment_id`, and a malformed deployment id throws `invalid_telemetry_collector_deployment_id`. `NodeRuntime.closeTelemetry()` drains the sink and `RunningServer.stop()` awaits that drain. Without the option the no-op sink is unchanged.

- Collector review repairs (#636): `exportProperties(event)` re-projects properties at the wire through the per-kind allowlist and the fixed exception classification, so a hand-built `ExportEvent` cannot export `agent_name`, other product facts, or raw exception names/codes. `EXPORT_EVENT_SCOPES` fixes each kind's scope (observations: workspace; `exception`: installation-only, `workspace_id` always null) and `validateExportEvent(event, identity?)` returns an `ExportEventInvalidReason` (`unregistered_kind`, `missing_trace_id`, `missing_event_id`, `missing_workspace_id`, `unauthorized_workspace_id`, `tenant_id_not_allowed`, `deployment_mismatch`, `component_mismatch`); the sink counts refused events in `stats().invalid` (`CollectorTelemetryStats`) and the HTTP transport refuses such a batch before sending. Non-`204` `2xx`, `507` and redirects are counted in `protocolErrors`, `storageFailures` and `redirects`. The bearer token must match `BEARER_TOKEN_PATTERN` (visible ASCII, no whitespace), else `invalid_telemetry_collector_token`; a deployment id equal to the replica hostname (`replicaHostnames`, by default `os.hostname()` and `HOSTNAME`) is refused with `invalid_telemetry_collector_deployment_id`. `resolveTelemetryDestination` / `TELEMETRY_DESTINATIONS` and `telemetryDestination` on `createNodeRuntime` / `startServer` select `none` or `collector` explicitly (`missing_telemetry_collector_config`, `unsupported_telemetry_destination`). New options `backoffMs` (500) and `retryAfterMaxMs` (30000, at most 30000). The batch being sent counts against `queueMax` / `queueMaxBytes` (`inFlight`, `inFlightBytes`); `413` split halves inherit the parent's delivery deadline; an attempt that outlives its timeout is abandoned even if the transport ignores `timeoutMs` (`attemptTimeouts`). `workspace_id` must match the native workspace-id grammar (`isArWorkspaceId`, exported as `workspaceIdOf`); observations with any other workspace text are counted `invalid` and never sent, and exceptions never carry a workspace id. At the shutdown deadline the batcher aborts the in-flight attempt through the new `BatchAttempt.signal` (the HTTP transport cancels its fetch), counts that batch once as dropped and ignores a late completion; no exactly-once delivery is implied.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Release notes bury the change

The new entries repeat the configuration and delivery specification, and this entry includes internal review history. AGENTS.md requires one short, impact-first bullet per user-visible change and says to omit internal review notes. Summarize the shipped behavior and link to the detailed guide before merging. The same pattern appears in CHANGELOG.md:27 and packages/engine/CHANGELOG.md:21, among other new entries.

Context Used: AGENTS.md (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/engine/CHANGELOG.md
Line: 24

Comment:
**Release notes bury the change**

The new entries repeat the configuration and delivery specification, and this entry includes internal review history. `AGENTS.md` requires one short, impact-first bullet per user-visible change and says to omit internal review notes. Summarize the shipped behavior and link to the detailed guide before merging. The same pattern appears in `CHANGELOG.md:27` and `packages/engine/CHANGELOG.md:21`, among other new entries.

**Context Used:** AGENTS.md ([source](https://github.com/agentworkforce/relaycast/blob/main/AGENTS.md))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Codex Fix in Claude Code

Comment thread docs/self-hosting.md
Comment on lines +161 to +164
The container image pins a published `@relaycast/engine`; until a release that
includes collector support is pinned there, setting
`RELAYCAST_TELEMETRY_COLLECTOR_URL` stops startup with exit code `64` and
`unsupported_telemetry_collector` rather than silently ignoring the setting.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Image support is described incorrectly

This paragraph says collector support requires updating the pinned npm release, but Dockerfile builds the checked-out engine and copies its dist over the installed package. An image built from this branch therefore includes collector support without changing that pin. Operators may wait for an unnecessary update or expect a startup refusal that will not happen. Describe support in terms of the selected image, and reserve unsupported_telemetry_collector for an engine that actually lacks the export.

Suggested change
The container image pins a published `@relaycast/engine`; until a release that
includes collector support is pinned there, setting
`RELAYCAST_TELEMETRY_COLLECTOR_URL` stops startup with exit code `64` and
`unsupported_telemetry_collector` rather than silently ignoring the setting.
The container image builds the engine from its checkout and replaces the
installed package's `dist`, so an image built from this source includes collector
support without changing the npm pin. Select an image that includes this support.
If its bundled engine lacks the collector export, setting
`RELAYCAST_TELEMETRY_COLLECTOR_URL` stops startup with exit code `64` and
`unsupported_telemetry_collector` rather than silently ignoring the setting.

Knowledge Base Used: Container deployment

Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/self-hosting.md
Line: 161-164

Comment:
**Image support is described incorrectly**

This paragraph says collector support requires updating the pinned npm release, but `Dockerfile` builds the checked-out engine and copies its `dist` over the installed package. An image built from this branch therefore includes collector support without changing that pin. Operators may wait for an unnecessary update or expect a startup refusal that will not happen. Describe support in terms of the selected image, and reserve `unsupported_telemetry_collector` for an engine that actually lacks the export.

```suggestion
The container image builds the engine from its checkout and replaces the
installed package's `dist`, so an image built from this source includes collector
support without changing the npm pin. Select an image that includes this support.
If its bundled engine lacks the collector export, setting
`RELAYCAST_TELEMETRY_COLLECTOR_URL` stops startup with exit code `64` and
`unsupported_telemetry_collector` rather than silently ignoring the setting.
```

**Knowledge Base Used:** [Container deployment](https://app.greptile.com/-/switch-tenant?tenant=agent-relay&next=%2Fcustom-context%2Fknowledge-base%2Fagentworkforce%2Frelaycast%2F-%2Fdocs%2Fcontainer-deployment.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Codex Fix in Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants