Skip to content

Add session telemetry - #1246

Open
burtenshaw wants to merge 4 commits into
ben/rfc008-l2-03-runtimefrom
ben/rfc008-l2-04-telemetry
Open

burtenshaw wants to merge 4 commits into
ben/rfc008-l2-03-runtimefrom
ben/rfc008-l2-04-telemetry

Conversation

@burtenshaw

Copy link
Copy Markdown
Collaborator

This PR adds authorized session telemetry for seed handling, rubric attribution and subject-emitted records. Part 4 of #1177; builds on #1181.

@burtenshaw burtenshaw added feature size: large Large pull request labels Sep 24, 2026 — with Cursor
@bot-ci-comment

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alignment Review Report

Scope: the diff for this PR only (git diff f84f8a31...434bb1dc), i.e. the "Add session telemetry" commit stacked on ben/rfc008-l2-03-runtime. This is the sanctioned PR4 — Session telemetry protocol slice of RFC-008.

Automated Checks

  • Lint: PASS (changed files). usort check, ruff format --check, and ruff check all pass on the 14 changed .py files. Repo-wide .claude/hooks/lint.sh reports 56 files needing formatting, but every one is pre-existing noise under envs/ plus the two known files tests/envs/test_grid_world.py / tests/envs/test_julia_env.py — none touched by this PR (consistent with AGENTS.md).
  • Debug code: CLEAN (this PR). .claude/hooks/check-debug.sh flags prints/TODOs only in pre-existing files (harbor/, cli/templates/, core/containers/test_local_docker_provider.py); none in the changed files.
  • Tests (local): green. 33 unit + 18 integration telemetry tests pass, including the security-critical integration/test_session_telemetry_protocol.py.

Open RFCs Context

  • RFC-008 Environment Auto-Validation — In Review (@zkwentz). Directly governs this change; the PR implements its PR4 slice and amends the RFC to document the wire protocol.
  • RFC-004 Rubric System. This PR extends the Rubric base contract (validation_config(), _evaluation_count) that RFC-004 defines.
  • Other open RFCs (000/001/002/003/005/012 In Review; 010/011 Draft) are not relevant to this diff.

Tier 1: Fixes Required

  • src/openenv/validation/runtime/collector.py:186 — the validation_open handshake calls telemetry_request(...) without try/except, unlike the validation_read path (wrapped at lines 276–296). If a subject replies to the open handshake with a non-string / oversized / non-JSON frame, the exception propagates to the outer handler (line 331) and the entire runtime collection returns with empty exchanges + failure_reason at failure_phase="validation_open", which runner._runtime then treats as a subject startup/runtime failure. Since telemetry is opt-in, orchestrator-only, best-effort evidence that "introduces no new passing grader by itself," a telemetry hiccup should degrade to telemetry_error while core reset/step/state evidence is still collected (symmetric with the read path). Suggested fix: wrap the open handshake and set telemetry_error on failure. (Confirm if the hard-fail is intended.)

Tier 2: Alignment Discussion

Principle Conflicts

None identified — the change actually reinforces the key invariants:

  • Agent isolation / dual-API boundary (INVARIANTS.md): telemetry is gated to ServerMode.SIMULATION + an explicit OPENENV_VALIDATION_TOKEN (32–256 chars), lives only on the orchestration /ws, and is verified not exposed via MCP or in production (test_production_mcp_has_no_telemetry_or_reset_tools, test_validation_is_opt_in_and_simulation_only). Credentials use SecretStr + secrets.compare_digest, are never echoed on error, and capabilities are per-connection (cross-socket / expired reads rejected).
  • Client-server separation: validation/runtime/* never imports core/env_server; the collector speaks the protocol via raw dicts.
  • Rewards in environment: rubric evaluation stays in the env; telemetry only reads introspection (safe config + last score) and performs no external reward computation.

RFC Conflicts

No hard conflict (this is RFC-008's own planned delivery), but two points warrant author sign-off:

ALIGNMENT FLAG: PR amends an In-Review RFC to define the wire protocol it implements

  • Principle/RFC at stake: RFC-008 (In Review); PRINCIPLES.md "decisions documented in RFCs should not change without an RFC"
  • The concern: the diff adds the "Session telemetry protocol (PR4)" section and replaces the prior "no new public wire messages" sentence. This matches RFC-008's stated stacked plan, so it's expected — but since the RFC is still In Review and authored by someone other than the PR author, the author should confirm the now-normative details (token provisioning, per-connection capability, snapshot schema, 100 steps / 202 ops / 8 MiB bounds) match intent before they harden into the contract.
  • Suggested reviewer: @zkwentz (RFC-008 author)

ALIGNMENT FLAG: New public Rubric introspection API + an internal RFC-008 inconsistency

  • Principle/RFC at stake: RFC-004 (Rubric system) and RFC-008 line 200 vs 613–614
  • The concern: this PR adds validation_config() and _evaluation_count to the Rubric base class (a new public contract on an RFC-004 abstraction), and deliberately excludes state_dict() from config exposure ("Arbitrary attributes and state_dict() are never serialized as configuration") — a sound security choice. But RFC-008 line 200 still says introspectability graders "read named_rubrics() / state_dict() / per-child scores." The two RFC passages should be reconciled, and the new base-class API blessed against RFC-004.
  • Suggested reviewer: @zkwentz (RFC-008), @Darktex (dual-API-boundary / core abstractions author)

Summary

  • 1 mechanical issue to fix (telemetry validation_open error-handling asymmetry — confirm if intended)
  • 2 alignment points for human review (RFC-008 self-amendment; new Rubric API + RFC-008/004 reconciliation)
  • 0 hard RFC conflicts (this is RFC-008's sanctioned PR4 slice)

Overall: clean, well-tested, and tightly aligned with the agent-isolation invariant. No blocking issues from the automated checks.

Open in Web View Automation 

Sent by Cursor Automation: Pre-review

Comment thread src/openenv/validation/runtime/collector.py Outdated
@burtenshaw
burtenshaw added this pull request to stack #1183 September 24, 2026 10:27
Comment thread tests/test_validation/integration/test_runtime_process.py Fixed

@k21993 k21993 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 317983cc4b55be8fa9a8fc97e00acefd554056c0, including the follow-up that preserves normal runtime evidence after a malformed telemetry-opening reply has been fully received.

I ran the focused telemetry and runtime suite: 57 passed. Ruff, Ruff format, usort, and git diff --check passed for the changed files. CI is green at this head, including the Linux Docker job. I did not run Docker locally on this macOS host.

I found no remaining blocker. Telemetry is opt-in and simulation-only, opens before measured operations, uses a connection-bound capability, excludes validation credentials from persisted evidence, bounds both the subject record and received snapshot, and does not interfere with normal reset, step, or state evidence.

This is the PR4 slice planned by RFC 008 and it keeps telemetry at the orchestration boundary rather than putting it in MCP. RFC 008 is still in review and this PR adds the Rubric.validation_config() contract, so signoff from the RFC or core owner is still appropriate.

@cursor cursor Bot mentioned this pull request Sep 25, 2026
16 tasks

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature size: large Large pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants