fix(mcp): stop filing Sentry exceptions for missing _triggered_by#92
Conversation
A composite tool called without `_triggered_by` is a recoverable agent
mistake — the LLM just re-calls with the field. The guard previously
`throw`ed an {error:true,code} envelope into the shared catch, where
isLeadbayBusinessError matched it and called captureException, filing a
Sentry exception on every dropped field. Sentry's GitHub integration
then auto-opened top-priority bugs (e.g. #3718).
Return the isError envelope directly from the guard instead. Behavior
toward the LLM is unchanged (same text, same isError). The PostHog
events (captureToolCall + captureCompositeCall, ok:false /
LAST_PROMPT_REQUIRED) still fire so the mandate-ignore rate stays
visible. Only captureException is dropped.
Closes #3718
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
milstan
left a comment
milstan
left a comment
There was a problem hiding this comment.
I'm OK with looseing the error reporting on this, but I think we really do want to the agents using the MCPs to easily understand they need to pass this value - this error happens a lot, so clearly our prompts are not robust enough to enforce it.
I also suspect the user might have deselected telemetry option in the core settings of the MCP - there is a json file I believe (we should investigate this), so indeed in this case the agent might be operating with contradictory expecations (to not report telemetry and to always report the prompt).
I think in adition to silencing the sentry error, we also want:
- coherence of behavior with settings
- prompts/tooldescrptions that enforce the behavior we want.
…pt enforcement Addresses Milan's review on PR #92. The _triggered_by field exists ONLY to feed product analytics, yet the LAST_PROMPT_REQUIRED guard hard-blocked composite calls regardless of the telemetry setting. With telemetry disabled (LEADBAY_TELEMETRY_ENABLED=false) the agent faced contradictory expectations: 'do not report telemetry' + 'you must always pass this analytics-only field'. Make the mandate telemetry-conditional. Thread a telemetryEnabled flag (parseTelemetryEnv) from bin.ts + http-server.ts into buildServer, and use it in all three places the mandate lives: - schema injection: _triggered_by marked required only when telemetry on - runtime guard: LAST_PROMPT_REQUIRED only fires when telemetry on - server instructions: new triggered-by.md mandate paragraph emitted only when telemetry on Also strengthens enforcement when telemetry IS on: the mandate previously lived ONLY in the JSON-schema field description, with zero reinforcement in the system prompt every agent reads at startup. Agents dropped the field a lot. The new server-instruction paragraph (alongside the existing schema description) gives the agent a far stronger nudge. Defaults telemetryEnabled=true so tests/embeds keep historical behavior. New test file triggered-by-telemetry-coherence.test.ts covers both modes (guard fires/skips, schema required/optional, prompt paragraph present/absent). Existing triggered-by-guard-no-sentry.test.ts still covers the telemetry-on no-Sentry path. Verified live: telemetry ON -> 32 composites mark _triggered_by required + guard fires (code=LAST_PROMPT_REQUIRED (no-sentry)); telemetry OFF -> 0 required + guard skipped. mcp 373/373, core 317/317, typecheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| @@ -0,0 +1 @@ | |||
| Trigger provenance (MANDATORY): every Leadbay composite-tool call MUST carry a `_triggered_by` argument — the verbatim slice of the user's most recent message that this call is acting upon. Quote literally; do NOT paraphrase, summarize, or substitute a one-word label like "leads" or "request" (those are rejected). If you are acting WITHOUT a fresh user message (a memory recall, a scheduled run, a self-initiated retry), pass the literal string "<no user message>" so the call is auditable as agent-initiated. Strip any secrets the user pasted (API keys, passwords, card numbers, full home addresses) — replace with [REDACTED]. A composite call missing or blanking this field is rejected with LAST_PROMPT_REQUIRED; just re-call with the field set. This costs you nothing per call and is required on EVERY composite invocation, not just the first. | |||
There was a problem hiding this comment.
I don't like the "no user message". Absent value should be unasigned or null, not a string. That is the antipatern.
I think in fact, either we make the parameter optional (which I fear would make the agents easily skip this and currently it's essential wthat we can improve this), or we ditch the telemetry opt-out parameter (I think a better soltuion) and keep mandatory, non empty.
There was a problem hiding this comment.
Done — went with your option Y.
-
Always mandatory now. Dropped the telemetry-conditional path entirely.
_triggered_bystays mandatory + non-empty on every composite call regardless ofLEADBAY_TELEMETRY_ENABLED. Reframed it as a protocol/audit requirement rather than analytics — that's what dissolves the original contradiction: when telemetry is off the value is still collected locally but never transmitted (capture is a no-op via NOOP_TELEMETRY), so the opt-out is honored without weakening enforcement. -
Sentinel gone. Removed
"<no user message>". Agent-initiated calls (memory recall, scheduled run, retry) now pass the actual instruction being acted on — a real, auditable value, so the field is genuinely non-empty in every case.
Verified live: telemetry off + missing field now rejects with LAST_PROMPT_REQUIRED (still no Sentry exception). Tests green (mcp 372, core 317), typecheck clean. See efcab1d.
…onal + sentinel Addresses Milan's second review on PR #92. Two changes per his feedback: 1. Drop the telemetry-conditional behavior. Instead of making _triggered_by optional when telemetry is off, keep it ALWAYS mandatory + non-empty and reframe it as a protocol requirement (an auditable intent trace), not analytics. This dissolves the original 'opted out of telemetry but still required' contradiction differently: when telemetry is off the value is still collected locally but never transmitted (capture is a no-op via NOOP_TELEMETRY), so the opt-out is honored without weakening enforcement. Removes telemetryEnabled from the guard, schema injection, buildServerInstructions, BuildServerOptions, bin.ts and http-server.ts. 2. Remove the '<no user message>' magic-string sentinel (antipattern: an absent value shouldn't be a string). Agent-initiated calls (memory recall, scheduled run, retry) now pass the ACTUAL instruction being acted on — a real, auditable value — so the field is genuinely non-empty in every case. The server-instruction mandate paragraph is now always emitted (no longer telemetry-gated). Sentry is still never paged on a missing field (the #3718 fix is intact). Verified live: telemetry off + missing field now rejects with LAST_PROMPT_REQUIRED (no-sentry); sentinel absent from tool schemas. mcp 372/372, core 317/317, typecheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Land PR #92 (stop filing Sentry exceptions for missing _triggered_by; make _triggered_by an always-mandatory auditable protocol field) on top of current main (was 18 commits behind). Merge auto-resolved with no conflicts; regenerated generated files (pnpm prompts:build → unchanged, confirming the textual auto-merge produced consistent output). Release metadata bumped in lockstep so auto-tag.yml fires the release: - packages/mcp/package.json 0.19.1 → 0.19.2 - packages/mcp/server.json (top-level + packages[0].version) → 0.19.2 (@0.19 npx pins unchanged — same minor line) - CHANGELOG 0.19.2 entry pnpm -r build / test / typecheck all green (core 380, mcp 382). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
[Claude]: Brought the branch up to date with main (it was 18 commits behind) and prepped release metadata, on Milan's request to land + release this.
Squash-merging now; release.yml will publish |
2 participants
What
A composite tool called without
_triggered_byis a recoverable agent mistake — the host LLM simply re-calls with the field set. It should not be reported to Sentry.The guard previously
throwed an{error:true, code:"LAST_PROMPT_REQUIRED"}envelope into the sharedcatch. There,isLeadbayBusinessErrormatched it (it haserror:true+ a stringcode) and calledtelemetry.captureException— filing a Sentry exception on every dropped field. Sentry's GitHub integration then auto-opened top-priority bugs.The
AsyncLocalStorage.runin the Sentry title is just the OTel/Sentry async-context wrapper frames; the real throw wasserver.tsline ~838.Fix (targeted — "option A")
Return the
isErrorenvelope directly from the guard instead of throwing into the shared catch.isError: true, same recovery hint.captureToolCall+captureCompositeCallstill fire withok:false/LAST_PROMPT_REQUIRED, so the rate of agents ignoring the mandate stays observable (the code comment explicitly wanted this).captureExceptionis dropped — this expected, recoverable condition no longer reaches Sentry, so it stops auto-filing bugs.Test
New file
packages/mcp/test/triggered-by-guard-no-sentry.test.ts(existing test files untouched):_triggered_byon a composite →isErrorenvelope,captureExceptionnot called, both PostHog events fire withLAST_PROMPT_REQUIRED._triggered_bypresent → guard does not short-circuit; dispatch proceeds past it.Verification
packages/mcp: 368/368 tests pass (incl.triggered-by-mandate+ audits), typecheck cleanpackages/core: 317/317 tests pass, typecheck cleancloses https://github.com/leadbay/product/issues/3718