[chagui]

Lands the review feedback that was prepared for #69 but did not reach main before the merge — plus the consensus P0/P1 list from #74's own review. The squash-merged #69 in main describes the pre-review-fix design (single PII gate, no environment_variable_to_clear, byte-boundary truncation, no UTF-8 validation, no late-path handling, silent argv/env truncation). This PR brings the implementation in line with the reviewers' agreed outcome.

Review-driven fixes applied this round

P0 (must fix before merge — codex held P0 for the cluster):

• Late-path span timestamps. lateRunSpan now stamps a zero-width span at handler-time (time.Now()) — start == end, both non-zero. Previously the span had epoch timestamps, which OTLP backends interpret as 1970 and either reject or display as off-the-flamegraph. Mirrors the populateFetchSpan pattern.
• Duplicate-event guard. Bazel's BEP contract is one ExecRequestConstructed per invocation, but BES gRPC reconnect/replay can violate it. New runEmitted bool on invocationState gates the late-path branch — a duplicate post-flush event now logs Warn and drops, instead of emitting a second bazel.run span with the same SpanID (which downstream backends collapse or reject). Pre-flush re-stamping is naturally idempotent so no separate guard is needed there.

P1 (3/3 reviewer consensus):

• Truncation records. applyExecRequestArgv, applyExecRequestEnv, and applyExecRequestEnvToClear now thread []truncationRecord through and append a record on cap. Pre-flush records ride writeRootSpan's existing root-span Warn loop; post-flush records are logged from handleExecRequestConstructed. Matches the applyPatternAttrs / appendGarbageMetrics pattern.
• bazel.run.argv_count (always-on). Parallels environment_variable_count so operators see the raw argv payload size even when the slice is capped or IncludeCommandArgs is closed. All three count attributes now report pre-cap totals.
• Post-flush log severity Debug → Info. handleExecRequestConstructed logs the late-path emission at Info (visible in production) instead of Debug (suppressed). Truncations during late emission also log at Warn.

P2 (consensus polish):

• Wire-order parity test. New TestExecRequest_WireOrderParity runs the same payload through both pre-flush and post-flush wire orders × {drop, build_only, targets, verbose} and asserts byte-identical bazel.run.* output — formalizes the design choice that bazel.run is filter-exempt because it carries displaced root-span data. A symmetric receiver-wide allowRun() gate is tracked separately as a follow-up; opening would create wire-order-dependent output, which is strictly worse.
• findRunSpans test helper. Replaces the hand-rolled four-level pdata iteration in TestExecRequest_LatePostFlush. Same shape as findFetchSpans.
• TestExecRequest_LatePostFlush asserts SpanID identity (spanIDFromIdentity(uuid, "run")) and non-zero zero-width timestamps — would have caught the P0 bugs immediately.
• TestExecRequest_LateDuplicateDropped. Regression guard for the new runEmitted flag.
• TestExecRequest_TruncationRecordsEmitted. Asserts the Warn-log contract via zaptest/observer, parallel to existing pattern-truncation tests.
• Cap rationale. Comment on maxRunArgvEntries / maxRunEnvEntries / maxRunEnvClearEntries cites observed shapes ("argv rarely exceeds 150 entries; OS-typical env has 30-100 entries").
• Per-value 256-byte truncation documented in docs/attributes.md (attribute table footer covers argv strings, env names/values, working_directory, env-to-clear names).
• env_count Debug log key renamed to env_var_count for parity with env_to_clear_count.

Lifted from #69 review (already in this PR pre-#74-review)

• Late post-flush handling via lateRunSpan (against silent drop)
• environment_variable_to_clear emitted with always-on count
• IncludeRunEnvironment PII gate split from IncludeCommandArgs
• UTF-8 validation on every bytes→string cast
• argv/env caps + per-value truncateAttrValue
• Server-side env sort

Tests

12 new across the two review rounds: late post-flush (existing), pre-BuildStarted drop, environment_variable_to_clear, non-UTF-8 skipped, argv-cap, env-cap, IncludeCommandArgs-only, IncludeRunEnvironment-only, late-duplicate dropped, wire-order parity (4 sub-cases), truncation records emitted. Total receiver/besreceiver count: 354.

Deferred to follow-ups

• bes_receiver_late_run_spans_total metric (new counter, separate concern from the trace path).
• Receiver-wide allowRun() symmetric filter gate. Single-side gating would create wire-order-dependent output; the test asserting parity is the placeholder until a coherent design lands.
• Cap-consts colocation across attrs.go (file-wide maxAttrValueLen) and invocation.go (per-feature caps). Cosmetic.

Cross-references #69 (the original feature) and the review summaries at /tmp/besreceiver-reviews/pr-69-review.md and /tmp/besreceiver-reviews/pr-74-review.md.

[chagui]

Round-2 review fixes amended (force-push pending).

P0 — runEmitted regression fixed. state.runEmitted = true now sits AFTER consumeAndRecord succeeds. On a retryable consumer error the handler returns the error (BES stream closes, Bazel reconnects, replays the event). Because the invocations map persists across stream reconnects, the previous early-commit would have silently dropped the replayed event. New regression test TestExecRequest_LateConsumerErrorDoesNotCommitGuard uses a flakyTracesConsumer to fail the first attempt and assert the second (replayed) attempt still emits the bazel.run span.

P1 — UTF-8 filter ordering symmetric across argv/env/env-to-clear. All three helpers now follow the same shape: UTF-8 filter raw bytes → sort (env / env-to-clear) → cap → emit. truncationRecord{original, kept} reflects post-UTF-8 valid count, so the three numbers (*_count, original, slice length) are now mutually consistent. The always-on *_count attrs still report the raw payload size — operators derive the UTF-8 skip count from the gap between count and Warn original. Switched to utf8.Valid([]byte) to avoid the allocation utf8.ValidString(string(b)) would force.

P1 — maxRunEnvEntries 50 → 150. Comment claimed 1.5-2× typical but 50 was below the median CI runner (30-100). Bumped to 150, comment now reads "1× typical at the high end with headroom; truncationRecord-driven Warn fires whenever the cap clips so operators with concrete data can argue for a higher value." Tests rewritten to derive over-cap inputs from maxRunEnvEntries + 30 (and matching for argv/clear) so further bumps stay in sync without test churn.

P2 polish (sort.SliceStable, argv [:n:n], drop-level test, late-span counter, appendBytesAsStringSlice helper, Warn-message unification) intentionally deferred — none change behaviour.

355 tests + lint green at 9402727. Cross-references /tmp/besreceiver-reviews/pr-74-review-round2.md.