Skip to content

Send the event when report() is given null options - #10

Open
dmccoystephenson wants to merge 2 commits into
mainfrom
feat/fix-null-report-options
Open

dmccoystephenson wants to merge 2 commits into
mainfrom
feat/fix-null-report-options

Conversation

@dmccoystephenson

Copy link
Copy Markdown
Member

Summary

  • report(name, options) now treats a null (or any non-object) options as "no options" before reading value/tags. Previously a null from untyped JavaScript threw a TypeError inside the serialize try, so the event was dropped and debug received a misleading could not serialize line.
  • A regression test was added to the TraceClient block: report("startup", null | 42 | "loose") delivers {application, name, tags: {version}} three times with an empty debug log.
  • A bullet was added under ## [Unreleased] in CHANGELOG.md.

Closes #5

Cross-client parity

No cross-client-visible behavior changed: the wire format, opt-out values and disabledReason strings are untouched. Whether trace-client-python and trace-client-java already send the event in their equivalent "null options/tags" case could not be checked from this session's sandbox (reads of the sibling repos were not permitted); that check is left to the reviewer, as #5 suggests. The JS fix is independently correct either way.

Test plan

  • CI test job (npm ci, npm run typecheck, npm test on Node 22) green on the PR head
  • Regression evidence: the new test is shown to fail with the fix reverted (CI-based temporary revert, since Node is not available in the dispatch sandbox)

Local verification was not possible: node is not installed in the sandbox this PR was prepared in, so the local anchor is UNVERIFIED and CI on the exact head SHA is the anchor.

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

🤖 Generated with Claude Code


drafted by Claude on behalf of Daniel Stephenson

A null options object from untyped code threw inside the serialize try,
dropping the event with a misleading "could not serialize" debug line.
Treat null or any non-object options as no options.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dmccoystephenson
dmccoystephenson force-pushed the feat/fix-null-report-options branch from 5cf820a to 1563ecd Compare October 9, 2026 08:09
Destructuring options outside the try let a throwing getter escape
report(), breaking the never-throws promise. Move it back inside and
pin the behavior with a test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review rubric (head 2844218):

  • Scope: PASS — 3 files (trace-client.ts, tests/trace-client.test.ts, CHANGELOG.md), +34/-2; every hunk serves report(name, null) drops the event with a misleading 'could not serialize' debug line #5.
  • Tests-new: PASS — no new public API; two new tests in the TraceClient block.
  • Tests-fix: PASS — node is absent in the dispatch sandbox, so the fallback ladder's rung 1 (CI-based temporary revert) was used: commit 5cf820a (TEMP: revert 1563ecd …) reverted only trace-client.ts; CI run 37903105614 went red with exactly not ok 14 - sends the event when untyped code passes null or a non-object as options (55/56 pass). The branch was then reset to 1563ecd and force-pushed; CI run 37903193300 went green (56/56).
  • Sibling structure / renames: PASS — no new files, no renames.
  • Docs: PASS — no README or TSDoc statement about report() changed meaning; CHANGELOG.md gained a bullet under [Unreleased].
  • Issue resolution: PASS — the options.value / options.tags reads named in report(name, null) drops the event with a misleading 'could not serialize' debug line #5 now go through a null/non-object guard.
  • CI: PASS — run 37903295667 on 2844218: # tests 57, # pass 57, # fail 0.

Repo-specific:

  • Zero-dependency: PASS — grep '^import' trace-client.ts empty; the only test import added is import type { ReportOptions } from "../trace-client.ts".
  • Edge gate intact: PASS — tsconfig.json untouched; npm run typecheck green in CI.
  • Never-throws: initially FAIL, fixed in 2844218 — commit 1563ecd placed the destructure before the serialize try, so a throwing getter on options would have escaped report() synchronously (on main that read was inside the try). The destructure was moved inside the try, and does not throw when reading the options throws pins it. That this test fails at 1563ecd is reasoned (the destructure was outside the try there), not run.
  • No console / export surface: PASS — no console., no new export.
  • Version agreement: PASS — no version change in this PR. Separately, CHANGELOG.md has no 0.4.1 heading even though TRACE_CLIENT_VERSION is 0.4.1; that is pre-existing and filed as its own issue rather than fixed here.
  • Cross-client parity: noted in the PR body. Whether the Python/Java clients behave the same way could not be checked from this sandbox.

Merge is held: this run is not authorized to merge, and the change touches report() behavior in trace-client.ts. A human needs to review it.

No local verification was possible. node is not installed in the sandbox, so CI on the exact head SHA served as the anchor. CI runs only Node 22 on ubuntu, but the change uses no new runtime API.

This review was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).


drafted by Claude on behalf of Daniel Stephenson

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.

report(name, null) drops the event with a misleading 'could not serialize' debug line

1 participant