T8: logException domain and skipFrames

[krassx]

Review-only PR. This change is already on main (the repo works directly on main; see CLAUDE.md). The base branch is the commit it forked from, so the diff below is exactly this task. Merging this PR changes nothing. Review findings become follow-up commits on main. Plan: docs/design/mobile-api-alignment.md.

Mobile parity: Android ExceptionOptions.Domain/SkipFrames, iOS exceptionDomain, mergingRules.skipFrames.

• skipFrames: marks the top N frames of the outermost exception hidden; the worker's managed path already skips hidden frames when picking the grouping location.
• domain: sent as exception.domain.
• mergingRules only carries skipFrames (flat option is the equivalent); includeVideo is N/A (no video).

Review focus / known gaps

• domain has no effect yet: the worker's JavaScript path (crash/managed/common.py) ignores it, unlike Android/Apple. Needs a small worker change.
• hidden-frames grouping behaviour verified from worker source only.
• One mutation (domain.trim()) survived initially; the test was strengthened but the mutation was not re-run.

🤖 Generated with Claude Code

[cursor]

Review: T8 logException domain and skipFrames

Reviewed e4763b2 (feat(core): logException domain and skipFrames options) at 752bd01, against logException / buildCrashJson / isUserFrame / callSiteFrames, the Android-canonical notes in docs/design/mobile-api-alignment.md, and the existing grouping invariant in crash.ts / stack.ts (worker managed/common.py:88).

Findings

P1 High — skipFrames can hide every grouping-eligible frame, including the last user frame. That is the same failure mode this SDK already shipped as SBROWSER-32/35/38/41 (frames: [] → no top frame → no grouping signature → one issue per event). The new test at client.test.ts explicitly blesses hiding all frames.

P2 Medium — Public JSDoc describes skipFrames as skipping the wrapper that called logException. The implementation skips the exception stack's top frames. For the common catch (e) { logException(e, { skipFrames: N }) } pattern those are the throw site, not the wrapper (the wrapper is not on e.stack).

Residual (not a code defect in this PR)

domain is sent as exception.domain (Android wire shape) but the JS worker path still ignores it. The design doc records this. The public JSDoc still says domain “organises issues,” which is not true for JS reports until the worker change lands.

Overall risk: Medium

Merge recommendation: Request changes

Clamp skipFrames so at least one user frame stays unhidden, and rewrite the JSDoc to match exception-stack semantics. domain can ship as a wire field once the JSDoc stops claiming grouping it does not have.

Most important to fix

1. Do not hide the last user !== false frame (applyExceptionOptions).
2. Change the skipFrames: 99 test that currently requires every frame hidden: true.
3. Document that skipFrames applies to the thrown value's frames, not the logException call site.

What looks good

Validation (non-string/empty domain, non-integer/NaN/negative skipFrames) is fail-open: invalid values warn through onError and the report still uploads. domain / hidden survive stampCrashProvenance and the zip (client.test.ts reads crash.json from the assembled bundle). Frames are marked hidden rather than dropped, which is the right JS stand-in for worker mergingRules.skipFrames. includeVideo / a nested mergingRules object are documented kept differences, not silent omissions.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

P1 High — skipFrames can hide every grouping-eligible frame

Problem: This loop hides frames[0 .. skipFrames) with no floor. skipFrames >= frames.length (and the skipFrames: 99 test) marks every frame hidden: true. _get_crash_location is documented to skip hidden frames; stack.ts / crash.ts already record that the worker emits no grouping signature without a top frame (managed/common.py:88). This is the same hole as empty frames: [] (SBROWSER-32/35/38/41: one new issue per event).

It is not only the 99 case. isUserFrame already marks node: / @bugsee/ as user: false because those frames were feeding grouping. After a documented wrapper skip, the leftover is often exactly those frames.

Impact: Wrapper authors who pass skipFrames lose grouping. Unrelated events become separate issues (no location) or collapse onto a shared node:internal frame.

Scenario:

function report(err: unknown) {
  void client.logException(err, { skipFrames: 1 }); // JSDoc: skip the wrapper
}
await report('payment failed'); // non-Error → callSiteFrames already stripped logException

Synthetic / async stacks are commonly two frames: report (user) + processTicksAndRejections (node:internal, user: false). skipFrames: 1 hides report. Nothing grouping-eligible remains. The existing test logException('str', { skipFrames: 1 }) only asserts frames[0].hidden and never that a user frame is still unhidden. A 1-frame new Error() constructed inside the wrapper fails the same way.

Fix: Clamp so at least one user !== false frame stays unhidden:

const frames = crash.exception.frames;
const lastUser = frames.findLastIndex((f) => f.user !== false);
const n = lastUser < 0 ? 0 : Math.min(options.skipFrames, lastUser);
for (const frame of frames.slice(0, n)) frame.hidden = true;

Replace the skipFrames: 99 assertion that currently requires every frame hidden. Keep skipFrames: 0 as a no-op.

[cursor]

P2 Medium — JSDoc describes the logException caller; the code skips the exception stack

Problem: This comment says to use skipFrames when a wrapper/adapter logs on the app's behalf, so errors do not group at the wrapper. applyExceptionOptions hides crash.exception.frames[0..N) — the thrown value's frames, V8-order (throw / new Error() site first). A wrapper that does logException(caught) does not appear on caught.stack. This repo's own adapters (express, fastify, reportError, …) correctly pass the original error with no skipFrames.

Impact: Following this JSDoc on the common catch path hides the fault site. Remaining frames are often a shared caller (Express Layer.handle, React render). Unrelated bugs collapse into one issue — the opposite of the stated goal.

Scenario:

app.use((err, _req, _res, next) => {
  void bugsee.logException(err, { skipFrames: 1 }); // “skip this middleware”
  next(err);
});

err.stack is the route that threw. Frame 0 (the actual bug) is marked hidden. Grouping moves to the next frame, frequently a framework frame shared by every request.

skipFrames does skip the wrapper when the wrapper constructs the error (logException(new Error('x'), { skipFrames: 1 })) or logs a non-Error (synthetic frames from callSiteFrames). That is the Android ExceptionSignature meaning; it is not “skip the function that called logException.”

Fix: Document exception-stack semantics, e.g. “Hides the top N frames of the thrown value when picking the grouping location. Use when this call constructs the Error (or logs a non-Error) so grouping does not sit on this helper. Logging a caught error does not skip this call site — it is not on that stack.”

[krassx]

Review findings triaged against current main (this PR branch is a frozen snapshot, so its diff does not show the fixes): skipFrames capping was fixed on main earlier; domain/skipFrames JSDoc corrected (7da7f3e). domain still has no grouping effect for JavaScript reports (worker-side follow-up).

The overview PR #46 (head = main) was re-reviewed at ba608c4: no remaining findings, recommendation Approve.

🤖 Generated with Claude Code

[krassx]

Closing instead of merging: this review-only branch is a frozen snapshot that now conflicts with the review base branch after #37/#40/#43/#46 were merged. Its content is already on main, and every finding was addressed there (see the earlier triage comment; #46 was approved at the final review).

🤖 Generated with Claude Code