Skip to content

feat(sentry-monitor): bundle Sentry error monitoring into the fork#3

Closed
insomnius wants to merge 2 commits into
boonfrom
worktree-sentry-bundled-extension
Closed

feat(sentry-monitor): bundle Sentry error monitoring into the fork#3
insomnius wants to merge 2 commits into
boonfrom
worktree-sentry-bundled-extension

Conversation

@insomnius

@insomnius insomnius commented Jun 2, 2026

Copy link
Copy Markdown

Summary

Bundles Sentry error monitoring directly into the fork as a default-on (DSN-gated) plugin, instead of the separate git-installed @getboon/openclaw-monitor. It forwards every error-bearing OpenClaw lifecycle event to Sentry and stays a clean plugin tapping hooks (no Sentry code in core src/**), honoring the plugin-agnostic-core rule.

  • New extension extensions/sentry-monitor/enabledByDefault: true, activation.onStartup, but inactive (safe no-op) until BOON_SENTRY_DSN or plugin-config dsn is set.
  • Captures: model_call_ended (error), agent_end (fail), after_tool_call (error), message_sent (fail), subagent_ended (error/timeout/killed/reset/deleted), cron_changed (run/delivery error), session_end (unknown), plus node uncaught/unhandled rejections. Errors only; no prompt/message/tool content.
  • Ships in core dist (not in the package.json files exclude list); @sentry/[email protected] mirrored at root as an internalized bundled-plugin runtime dep (same pattern as bonjour/@homebridge/ciao).
  • Docs at docs/gateway/sentry-monitor.md (+ nav), plugin: sentry-monitor labeler entry.

Design / typing

  • Pure per-hook capture builders returning a discriminated SentryCapture union; dispatch typed against a minimal structural SentryCaptureClient (no cast, mockable).
  • Hook payloads use the real openclaw/plugin-sdk/types (PluginHook*Event) rather than local mirrors, so a core field rename/removal is a compile error rather than a silently-undefined Sentry field.
  • A single documented coerceHookEvent boundary instead of scattered as unknown as.

Tests

44 unit tests across format, captures, dispatch, and register: every hook trigger/no-trigger boundary, dispatch routing (exception vs message, null no-op), and the register() activation paths (no-DSN inactive → no init/no hooks; env + config DSN → init + all 7 hooks + flush lifecycle).

Verification

node scripts/run-vitest.mjs extensions/sentry-monitor   # 44 passed (4 files)
node scripts/run-tsgo.mjs -p tsconfig.extensions.json                       # exit 0
node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.extensions.test.json    # exit 0
pnpm build                                              # clean; emits dist/extensions/sentry-monitor/
node scripts/root-dependency-ownership-audit.mjs --check  # ok (@sentry/node classified as bundled-plugin runtime)

Real behavior proof

Behavior addressed: Sentry error reporting ships with the fork by default (DSN-gated), as a bundled plugin rather than a separate git install.
Real environment tested: local build + unit/type verification in a worktree off boon. Confirmed the plugin compiles into dist/extensions/sentry-monitor/ and is discovered by the bundled-plugin build pipeline.
Exact steps or command run after this patch: the five commands above (vitest, two tsgo lanes, pnpm build, dependency-ownership --check).
Evidence after fix: 44/44 tests pass; both tsgo lanes exit 0; full pnpm build clean with dist/extensions/sentry-monitor/{index.js,index.d.ts,openclaw.plugin.json,package.json} emitted; dep audit ok.
Observed result after fix: bundled plugin builds, typechecks, and is wired with default-on + DSN-gated activation.
What was not tested: no live gateway run with a real BOON_SENTRY_DSN delivering an actual event to a Sentry project; no end-to-end assertion that a triggered error appears in Sentry (logic is covered by unit tests against a mocked client). Channel connect/disconnect capture is intentionally out of scope (needs a new core channel_connection_changed hook — follow-up).

🤖 Generated with Claude Code


Summary by cubic

Bundle a Sentry error monitor as a built‑in plugin. It forwards error-only OpenClaw lifecycle events to Sentry without touching core code.

  • New Features

    • Bundled extensions/sentry-monitor/ with startup activation, shipped in core dist.
    • Captures error-bearing hooks: model calls, agent ends, tool calls, message send failures, subagent ends, cron errors, abnormal session ends; plus Node uncaught exceptions/unhandled rejections. Metadata only (no prompts/messages/tool payloads).
    • Adds @sentry/[email protected] as a bundled-plugin runtime dep; docs at docs/gateway/sentry-monitor.md. Knip updated to ignore @sentry/node.
    • Registers via typed api.on(...) per bundled-plugin boundary; added to contracts allowlist and fixed docs lint.
  • Migration

    • Set BOON_SENTRY_DSN or plugin config dsn to enable.
    • Optional: environment (defaults to hostname), tracesSampleRate (defaults to 0).
    • No DSN = safe no-op; no hooks are registered.

Written for commit 26ab8f6. Summary will update on new commits.

Review in cubic

Adds a bundled, default-on (DSN-gated) plugin that forwards every
error-bearing OpenClaw lifecycle event to Sentry: model_call_ended,
agent_end, after_tool_call, message_sent, subagent_ended, cron_changed,
and abnormal session_end, plus node-level uncaught/unhandled rejections.
Inactive (safe no-op) until BOON_SENTRY_DSN or plugin-config dsn is set.

Vendored from @getboon/openclaw-monitor and adapted for in-fork bundling:
ships in core dist, @sentry/node mirrored at root as an internalized
bundled-plugin runtime dependency.

Strictly typed: pure per-hook capture builders return a discriminated
SentryCapture union; dispatch is typed against a minimal structural
client; hook payloads use the real openclaw/plugin-sdk/types so core
field changes surface at compile time. Extensive unit tests cover every
trigger boundary, dispatch routing, and the register() activation paths.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed

Re-trigger cubic

- Register lifecycle hooks via the typed api.on(...) API instead of raw
  api.registerHook(...), per the bundled-plugin boundary contract; add the
  registration file + its hook names to the contract allowlist. This also
  removes the cast/generics (api.on supplies per-hook typed events).
- Drop the redundant String() coercion in pruneTags (values are already
  typed string) and the now-moot coercion test; use mock.lastCall to avoid
  an unnecessary non-null assertion in dispatch tests — clears 5 oxlint errors.
- Ignore @sentry/node in both knip bundled-dependency lists (root + extension),
  matching the @homebridge/ciao bundled-at-root precedent.
- Add the blank line markdownlint MD031 requires around the config fence.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@insomnius

Copy link
Copy Markdown
Author

Superseded by #4, which recreates this work as a single GPG-signed commit (-S) authored as [email protected]. Same tree and same green checks; closing this one.

@insomnius insomnius closed this Jun 2, 2026
@insomnius
insomnius deleted the worktree-sentry-bundled-extension branch June 2, 2026 13:21
insomnius added a commit that referenced this pull request Jul 6, 2026
…lee-level test

Round-3 review findings on PR #39:

- `docs/channels/msteams.md` "Set to \`false\` explicitly on any
  deployment that overrides this default and wants to opt out." leaked the
  existence of a fork override to the upstream docs mirror while sitting
  next to "Default: false.". Drop the trailer; the default sentence is
  enough for upstream readers, and the fork `CHANGELOG.md` still
  documents the fork override + opt-out.
- `extensions/msteams/src/monitor-handler/inbound-media.test.ts:231`
  test title said "(upstream default)", but the assertion is at the
  callee boundary and stays valid regardless of the caller's default. The
  message handler applies the resolver upstream, so this test proves the
  callee contract, not the caller default. Retitle + rewrite the leading
  comment to describe the contract, not the (now-misleading) upstream
  behavior.

Round-3 finding #3 (PLAUSIBLE, not filed): no message-handler-level
end-to-end test that exercises the fork default. Accepted as-is —
resolver unit tests + explicit callee-level tests + comment coverage give
enough confidence for a 1-line default flip. Full message-handler test
scaffolding is heavy and out of scope for this PR.
insomnius added a commit that referenced this pull request Jul 7, 2026
)

* fix(msteams): default alwaysFetchGraphMessage=true on the Boon fork

Every Teams tenant in the Boon fleet observed the ENG-14349 Bot Framework
stub-stripping regression - inbound activities arrive without the
`<attachment id=...>` HTML stub and without `reference`-typed entries
even with RSC consent granted, so attachments silently vanish unless the
Graph re-fetch path fires.

Upstream 6.11 already ships the fix as opt-in config
`channels.msteams.alwaysFetchGraphMessage` (default off, kept off upstream
so tenants where Bot Framework delivers the stub correctly don't pay
per-message Graph latency). The Boon fleet does not have such tenants -
every Teams host was previously relying on a host-local msteams SP-PATCH
baked in via gandalf-manager `scripts/fleet-msteams5.22-bump.sh`.

Flip the fork default via a small resolver so an explicit `false` still
opts out per host, and the change is unit-testable without wiring through
the full message-handler.

Version bump: 2026.6.11-boon.1 -> 2026.6.11-boon.2.

* fix(msteams): align config docs + shrinkwrap with fork default flip

Round-1 review findings on PR #39:

- `npm-shrinkwrap.json` still pinned `2026.6.11-boon.1`; regen via
  `scripts/generate-npm-shrinkwrap.mjs` so release-check version parity
  (`scripts/check-openclaw-package-tarball.mjs:243`) passes.
- Config-contract docs at `src/config/types.msteams.ts:164` and
  `src/config/zod-schema.providers-core.ts:1651` still asserted
  "Default false." — now note upstream=false / fork=true so the schema/type
  peer stays aligned with the runtime resolver, per root `CLAUDE.md`
  "Config contract: exported types, schema/help, metadata, baselines, docs
  aligned."

* docs(msteams): document alwaysFetchGraphMessage + sync inbound-media JSDoc

Round-2 review findings on PR #39:

- `extensions/msteams/src/monitor-handler/inbound-media.ts:37` — the
  sibling JSDoc on `resolveMSTeamsInboundMedia`'s param still said
  "Default false (upstream behavior)." and is what editors surface at the
  message-handler call site. Note that the caller resolves the fork default
  before calling in.
- `docs/channels/msteams.md` — `alwaysFetchGraphMessage` was never
  listed in the user-facing Configuration section even upstream; add it so
  the flag is discoverable and the CHANGELOG's opt-out (`false`) is
  findable via docs. Doc source publishes to the openclaw/docs upstream
  mirror, so the entry stays upstream-neutral and describes upstream
  behavior; the fork-specific default is called out only in the fork
  `CHANGELOG.md` and in the internal-facing schema/type JSDoc.

* docs+test(msteams): tighten alwaysFetchGraphMessage doc + retitle callee-level test

Round-3 review findings on PR #39:

- `docs/channels/msteams.md` "Set to \`false\` explicitly on any
  deployment that overrides this default and wants to opt out." leaked the
  existence of a fork override to the upstream docs mirror while sitting
  next to "Default: false.". Drop the trailer; the default sentence is
  enough for upstream readers, and the fork `CHANGELOG.md` still
  documents the fork override + opt-out.
- `extensions/msteams/src/monitor-handler/inbound-media.test.ts:231`
  test title said "(upstream default)", but the assertion is at the
  callee boundary and stays valid regardless of the caller's default. The
  message handler applies the resolver upstream, so this test proves the
  callee contract, not the caller default. Retitle + rewrite the leading
  comment to describe the contract, not the (now-misleading) upstream
  behavior.

Round-3 finding #3 (PLAUSIBLE, not filed): no message-handler-level
end-to-end test that exercises the fork default. Accepted as-is —
resolver unit tests + explicit callee-level tests + comment coverage give
enough confidence for a 1-line default flip. Full message-handler test
scaffolding is heavy and out of scope for this PR.

* fix(msteams): drop extensions/ path from core config JSDoc

CI `checks-fast-contracts-channels-a` failure: the round-1 doc updates
inserted the plugin-private path
`extensions/msteams/src/monitor-handler/graph-fallback-default.ts` into
core `src/config/types.msteams.ts` and `src/config/zod-schema.providers-core.ts`
JSDoc, which triggered
`src/channels/plugins/contracts/channel-import-guardrails.test.ts`
("keeps core production files off plugin-private src imports").

Reword the doc so it names the fork consume-site by plugin
(`@openclaw/msteams`) and describes the runtime seam (resolver applied
before `resolveMSTeamsInboundMedia`), rather than pointing at a
plugin-private source path. Verified locally:
- `channel-import-guardrails.test.ts`: 148 tests passing.
- `graph-fallback-default.test.ts` + `inbound-media.test.ts`: 16 tests
  passing.
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.

1 participant