feat(spx-gui): add Sentry tracing for SPX runtime and AI interactions - #3383
1034674309 wants to merge 6 commits into
Conversation
Add Sentry integration to ProjectRunner to capture and report runtime errors from tools/ispx with structured context including file, line, function, and column information. Key features: - Parse panic JSON logs from tools/ispx console output - Report runtime panics to Sentry with SpxRuntimePanic exception type - Add structured context (file, line, column, function) to events - Instrument lifecycle spans for engine init, build, start, and stop - Deduplicate errors within 5-second window to prevent duplicate reports - Graceful degradation when Sentry is unavailable - Async reporting to avoid interfering with WASM panic unwinding Closes goplus#2419 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Review summary
This change adds Sentry tracing spans around SPX runner lifecycle operations and structured SPX runtime-panic reporting. The overall structure is clean, parseRuntimePanic is strict and defensive at the iframe trust boundary, and the WASM-unwinding deferral (setTimeout(0)) is a subtle correctness detail handled well. The four review passes surfaced a few concrete concerns worth addressing, noted inline. No blocking issues; submitting as a non-approving comment review.
Highlights:
- Performance:
JSON.parseruns on every single-stringconsole.logfrom the game loop — worth a cheap prefilter. - Correctness: grace-period (1000ms) vs recent-panic window (5000ms) mismatch, and panic state is not reset on
stop(), so a stale panic can suppress a legitimate error in a subsequent run. - Security (low): attacker-controlled panic strings reach Sentry unbounded.
|
|
||
| let value: unknown | ||
| try { | ||
| value = JSON.parse(args[0]) |
There was a problem hiding this comment.
Performance: parseRuntimePanic runs on every single-string console.log forwarded from the game iframe, and the only guard before JSON.parse is the arity/typeof check on line 244. A game doing console.log("score: 42") once per frame will trigger a JSON.parse + throw/catch at up to 60x/sec. Consider a cheap prefilter before parsing, e.g. skip unless the string starts with { and contains "panic" — the payload requires record.msg === 'panic' anyway, so this is behavior-preserving.
|
|
||
| function hasRecentRuntimePanic() { | ||
| const panic = lastRuntimePanic | ||
| return panic != null && Date.now() - lastRuntimePanicAt <= 5000 |
There was a problem hiding this comment.
Magic-number mismatch / correctness: this bare 5000 recent-panic window is inconsistent with runtimePanicGracePeriod = 1000 (line 185). The deferral only waits 1s for a future panic, but a panic that arrived up to 5s earlier suppresses the generic capture. Recommend extracting both into named constants and documenting why they differ (or unifying them). A raw literal here is easy to drift out of sync with the grace period.
| } | ||
|
|
||
| function deferRuntimeErrorCapture(err: unknown, ctx: string) { | ||
| if (hasRecentRuntimePanic()) return |
There was a problem hiding this comment.
Cross-run stale-panic suppression: lastRuntimePanic/lastRuntimePanicAt are reset only at the start of runInternal (lines 429-430), not in stop(). During rerun() (which calls stop() then run()), and for up to 5s after any run ends, hasRecentRuntimePanic() can still return true from a previous run's panic. That would cause this early return to silently drop a legitimately different error from the new run. Consider resetting panic state on stop() as well, or keying "recent" to the current run/AbortController.
| line, | ||
| column | ||
| }) | ||
| Sentry.captureException(exception) |
There was a problem hiding this comment.
Security (low) — unbounded attacker-controlled data to Sentry: error, functionName, and file originate from the iframe's console.log (user/game WASM) and are forwarded verbatim into Sentry as the exception message, name, and context with no length cap or sanitization. A malicious project can emit arbitrary/large panic strings (log-poisoning) and could embed runtime-harvested data into your Sentry stream. Consider truncating these strings to a bounded length and confirming Sentry PII scrubbing covers this path.
| // eslint-disable-next-line no-console | ||
| console.log(...args) | ||
| const panic = parseRuntimePanic(args) | ||
| if (panic != null) handleRuntimePanic(panic.error, panic.functionName, panic.file, panic.line, panic.column) |
There was a problem hiding this comment.
Minor: parseRuntimePanic already returns a RuntimePanic object, but it's destructured into 5 positional args here only for handleRuntimePanic to immediately reassemble the identical object (line 215). Passing panic directly (handleRuntimePanic(panic)) is simpler and removes a class of positional-arg ordering bugs.
| }) | ||
| try { | ||
| const result = await operation() | ||
| span.setStatus({ code: 1 }) |
There was a problem hiding this comment.
Minor (readability): span.setStatus({ code: 1 }) / { code: 2 } use bare numeric codes for OK/ERROR. A short comment or named constants (// 1 = OK, 2 = ERROR) would match how logLevels is spelled out elsewhere in this file.
Create isolated Think and Archive traces, propagate per-call trace headers, record transport and command spans, and report terminal outcomes with bounded exception context. Consume structured backend failure metadata while keeping quota, cancellation, and exhausted 429 flows out of Sentry Issues.
|
This PR is superseded by split PRs so runtime monitoring and AI tracing can be reviewed independently:
SPX runtime panic monitoring is not part of the AI Sentry contract. It is left on this old branch and can land later as its own PR if still wanted. Closing this PR. Please review #3470 instead. |
Summary
Integrate Sentry with the SPX runtime and the AI interaction lifecycle tracked by #2419.
The runtime reports actionable panics without parsing every normal game log. AI interactions get isolated Think and Archive traces with per-call propagation headers, transport/command spans, explicit terminal outcomes, and one terminal exception only when the documented flow actually stops.
The implementation follows
tools/ai/SENTRY_PLAN.md;tools/ai/SENTRY_IMPLEMENTATION.mdmaps each contract item to its code and tests.Runtime Sentry foundation
The branch already contains the SPX runtime error bridge used by the AI integration:
Plan-by-plan verification
Slice 1: bridge and isolated Think trace
Each
Player.Thinkcreates a new Sentry root rather than inheriting the page/pageload trace:Implementation details:
startThinksynchronously starts an independent root and returnsSentry-Trace/Baggagebefore the fetch.setTimeout(0)so Sentry SDK work cannot interfere with Go WASM stack unwinding.Map<thinkId, state>rather than process-wide singleton variables.context.Context; timeout child contexts inherit the correct headers.endThinkis registered beforestartThink; bridge failures cannot leaveinteractionActivelocked.game_session_idgroups Think and Archive roots from the same game session.Transport attempts
Every
Transport.Interactcall createsai.transport.attemptwith:turn_indexattemptbackend_category,backend_reason, andrequest_idThe fetch carries only the current Think's trace headers. HTTP failures are decoded through typed errors rather than JavaScript string matching.
Command execution
Registered handlers create
ai.command.executecovering argument population and handler execution.command_name,turn_indexBreakfinishes successfully and ends the Think successfullyerror_message, and continue to the next turnmodel_quality_failure / invalid_argumentsAbortThreadterminate the Think asruntime_failure / handler_panicai.detail; they do not create a fake handler spanSlice 2: backend continuation
The transport propagation produces the complete tree with the backend HTTP transaction and provider span.
The companion backend PR is goplus/builder-backend#346. It provides:
ai.upstreamandai.archive.upstreamrequest_idfirst_meaningful_delta_msai.detailinput/output{category, reason, request_id}This frontend copies backend classification only to attempt-level
backend_*attributes. Backend categories are intentionally separate from frontend terminal categories.Slice 3: Think terminal state
think()records onethinkFinish; one deferredendThinkwrites the root terminal state and optionally captures one exception.outcomesuccessokBreaksuccessokcancelledcode=40301quota_exhaustedrate_limitedfailureerrorfailureerrorfailureerrorfailureerrorfailureerrorfailureerrorThe root also records
turn_countandattempt_count; failures include their frontend category/reason.429 is intentionally not an Issue source. It can be frequent, so exhausted 429 flows use
outcome=rate_limitedand error attempt spans only.Terminal exception payloads
Only
outcome=failureproducesAIThinkFailure.Categories and reasons:
transport_failure / timeouttransport_failure / networkmodel_quality_failure / missing_initial_commandmodel_quality_failure / invalid_argumentsmodel_quality_failure / turn_limitruntime_failure / handler_panicEach exception includes
game_session_id, category, and reason. Relevant failures add turn/attempt and command context. The current user message is retained, command name plus arguments are capped at 4 KiB, and error text is capped at 1 KiB.Capture, root attributes, span finish, and
Map.deleteall happen in the same deferred bridge task, preventing duplicate terminal reports.Independent Archive trace
Archive runs in its own owner-scoped
spx.Gocontext and starts a separate root:ai.archive.transport.attemptand backend classification attributes.AIArchiveFailurewitharchive_failure / retries_exhausted.empty_archivemay be retried; only exhaustion of the complete frontend Archive flow creates an Issue.outcomeattribute.Slice 4: backend contract consumption
The frontend recognizes and propagates the backend contract supplied by goplus/builder-backend#346:
403plus JSONcode=40301is quota exhaustionTooManyRequestsErrorand observesRetry-Aftercategory,reason, andrequest_idsurvive wrapped HTTP/timeout errorsDetailed provider/model reasons remain backend-owned and are copied verbatim to attempt spans. The frontend does not duplicate those reasons as Think exception categories.
Sampling
Independent
ai.*roots use an app-providedaiSampleRateinstead of inheriting page sampling.VITE_SENTRY_AI_SAMPLE_RATEis exposed for Account and XBuilder apps.1for validation and initial rollout.Explicitly out of scope
Tests and checks
cd tools/ai && go test ./...cd tools/ai && go vet ./...cd spx-gui && pnpm run type-checkcd spx-gui && pnpm run linttools/ispx/build.shgit diff --checkTests cover:
endThink/endArchiveManual validation
A successful AI-Town Think was verified end-to-end as an independent trace connected through the backend HTTP transaction. Failure, exhausted 429, and empty-archive behavior are covered by unit tests; they have not all been forced in a live game session.
Dependencies
tools/aiand transport code.Closes #2419.