fix(insight): fail open on NaN sampling_rate + on-change note

[wangyb-A]

What

Stacked follow-up on #632 (base branch feat/workflow-insight-plugin).

Fixes a correctness bug and adds a documentation note:

1. sampling_rate=NaN fails open to 1.0. NaN compares False to every
   operand, so a NaN rate flowed straight through _should_sample
   (rate >= 1 → False, rate <= 0 → False, x < NaN → False) and silently
   sampled out every execution, disabling all instrumentation.
   _resolve_sampling_rate now detects NaN and coerces it to 1.0 (full
   sampling), matching the JS plugin's treatment of non-finite/invalid rates.
   All other sampling behavior (clamping of out-of-range values, None handling) is
   unchanged.

2. README note that in on-change mode exporter calls currently run
   synchronously on the SDK checkpoint path, so slow exporters can delay
   workflow progress. Asynchronous scheduling/coalescing is deferred and
   tracked in [Feature]: Add workflow insight plugin #687.

Tests

• test_resolve_sampling_rate_nan_fails_open_to_one — _resolve_sampling_rate(float("nan")) == 1.0.
• test_nan_sampling_rate_emits_instead_of_silently_disabling — a plugin
  configured with sampling_rate=NaN still emits a record and surfaces the
  warning.

Validation

• Full insight suite (unit + local-runner e2e): 62 passed.
• hatch run types:check: Success, no issues.
• hatch fmt --check (ruff check + format): all checks passed, 17 files formatted.
• hatch build: sdist + wheel built.
• Commit-message linter (lintcommit.py): PASS.

Notes

• Draft, stacked on feat/workflow-insight-plugin (do not merge until feat: add Workflow Insight instrumentation plugin #632 lands).
• Parent PR: feat: add Workflow Insight instrumentation plugin #632
• Deferred async export scheduling/coalescing: [Feature]: Add workflow insight plugin #687

[github-actions]

Claude AI review

Reviewed the full diff (plugin.py, test_plugin.py, README.md) against the base revision.

Correctness of the fix: _resolve_sampling_rate previously let a NaN sampling_rate fall through to return float(rate) unchanged. Since NaN compares False to every operand, this made _should_sample return False for every execution (rate >= 1 → False, rate <= 0 → False, x < NaN → False), silently disabling all instrumentation. The new math.isnan(rate) branch, placed before the existing clamp logic, correctly intercepts this and coerces to 1.0 (full sampling), matching the JS plugin's fail-open behavior. I traced call sites (WorkflowInsightPlugin.__init__ → self._sampling_rate, used once in _sampled_in/_should_sample) and confirmed WorkflowInsightConfig.__post_init__ performs no validation that would intercept a NaN sampling_rate before it reaches this function, so the fix is reachable and effective. The isinstance(rate, float) and math.isnan(rate) guard is placed after the existing isinstance(rate, (int, float)) check, so it only runs on numeric input, avoiding a TypeError. Non-finite values other than NaN (e.g. inf, -inf) are unaffected and continue to be clamped by the existing rate < 0 or rate > 1 branch, so there's no regression to that path.

Tests: Both new unit tests exercise the fix meaningfully — one directly asserts _resolve_sampling_rate(nan) == 1.0, the other drives the plugin end-to-end with sampling_rate=NaN and confirms a record is still emitted (i.e., instrumentation isn't silently disabled). This matches the repo's testing requirements for an isolated bug fix (unit tests suffice; no e2e/integration test needed since the change is confined to one pure function's edge case).

Docs: The README addition is a plain informational note about synchronous exporter calls in on-change mode, linked to a tracked follow-up issue; it doesn't claim anything unverifiable against the current code.

Determinism/replay: sampling_rate resolution happens once per plugin construction from static config, not from any non-deterministic source, so this doesn't introduce a replay-determinism concern.

No functional, typing, or test-coverage issues found. One very minor stylistic nit (the isinstance(rate, float) re-check is redundant since math.isnan also accepts int) is not worth a formal comment — it has no behavioral impact. No residual test risk identified beyond pre-existing, unrelated behavior (e.g., inf/-inf clamping) which this PR does not touch.

Reviewed commit 2d540e27d4d4ff0d1d91ad6816e501f6e6e9470b. Workflow run

[github-actions]

Codex AI review

No actionable findings. Residual risk is low: NaN sampling is covered by unit-level hook simulation, but not through the local runner end to end.

Reviewed commit 2d540e27d4d4ff0d1d91ad6816e501f6e6e9470b. Workflow run