feat(telemetry): emit partner_nodes_detected + stamp caller_kind on every event (BE-5633)

[mattmillerai]

ELI-5

The CLI already figures out when your workflow uses a paid "partner" node (Veo, Kling, BFL, Gemini, …) — but it never told analytics about it, so nobody could see how often that happens or how often it fails for a missing key. This adds one event for that. It also stamps every telemetry event with who ran the command — a human at a terminal, a pipe, an agent, Claude Code, or a custom COMFY_USER_AGENT label — so agent-vs-human usage is finally measurable across the whole event stream.

Telemetry only: no change to what comfy run does, what it submits, or when it fails.

What changed

1. partner_nodes_detected (comfy_cli/command/run/__init__.py) — local comfy run fires it whenever the workflow uses partner-API nodes, with partner_nodes (sorted distinct class_types, capped at 20 to defend against pathological graphs), partner_node_count (exact, uncapped), where: "local", and credential_present. It fires before the partner_node_requires_credential rejection, so runs turned away for a missing key are still counted with credential_present: false — that funnel is exactly what the metric is for. The credential is resolved once and reused for both the event prop and the existing gate. _detect_partner_nodes stays a pure function; preflight.py is untouched.

2. caller_kind on every event (comfy_cli/tracking.py) — _dispatch now enriches with caller_kind alongside cli_version/tracing_id, computed once at module scope from detect_caller().kind (matching the existing tracing_id/cli_version pattern, so isatty isn't re-run per event). This lands on every event — execution_*, the new partner event, feedback — which is the point; it is purely additive, so no existing dashboard breaks.

No PII: class_types are node names, the same data workflow_unknown_nodes error details already send. track_event still no-ops without consent (DO_NOT_TRACK / COMFY_NO_TELEMETRY / no consent flag), so the new event is gated exactly like every other one.

Judgment call — ordering vs the spend gate (please sanity-check this one)

The plan said to fire the event immediately after _detect_partner_nodes(...). Since that plan was written, the BE-4326 spend gate landed between detection and credential resolution, and it is explicitly ordered to run before any credential resolution so a refusal never triggers a network OAuth refresh (there is a regression test asserting _resolve_partner_credential is not called: test_paid_node_machine_mode_fails_closed_without_flag).

credential_present needs that resolution, so firing the event pre-gate would have meant resolving the credential early and breaking that invariant. I kept the gate's invariant and placed the event immediately after it, still before the credential-rejection branch (the ordering the plan actually justified).

Consequence, stated plainly: a run refused for lack of --allow-spend emits no partner_nodes_detected. The population that reaches the event is "partner run where spending was consented to." If the spend-declined funnel is also wanted, it deserves its own event (e.g. on spend_consent_required) rather than an early credential resolve here — happy to file that as a follow-up. test_does_not_fire_when_the_spend_gate_refuses documents the boundary so it can't drift silently.

Scope notes

• Only the local execute() path fires the event, per the ticket's where: "local" prop. execute_cloud() also detects partner nodes and is deliberately left alone — cloud usage is already measurable server-side, and adding a where: "cloud" variant is a separate call.
• caller_kind is computed at import, so it reflects the process's TTY-ness at startup rather than at each event. That is intentional (and cheaper); a later sys.stdout swap won't change it.

Tests

• tests/comfy_cli/test_tracking.py — caller_kind reaches provider .track properties for both track_event and submit_feedback; is one of the intrinsic kinds by default; an explicit COMFY_USER_AGENT label flows through lowercased. Also updated the existing exact-key-set assertion in test_properties_default_to_empty_dict.
• tests/comfy_cli/command/test_run.py — new TestPartnerNodesDetectedTelemetry: fires with the right props under --api-key and under an env key; does not fire for a partner-free workflow; does fire when the run is then rejected with partner_node_requires_credential (credential_present: false); does not fire when the spend gate refuses; the 20-node cap truncates the list while partner_node_count stays exact.

Verification

ruff check . + ruff format --diff . clean on the CI-pinned ruff 0.15.15, and the full pytest suite: 3752 passed, 37 skipped.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 30 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3450ff80-93d4-4393-8a35-f8091cda34e2

📥 Commits

Reviewing files that changed from the base of the PR and between d3220c9 and 9fb8218.

📒 Files selected for processing (10)

• comfy_cli/caller.py
• comfy_cli/cmdline.py
• comfy_cli/command/run/__init__.py
• comfy_cli/output/renderer.py
• comfy_cli/tracking.py
• docs/json-output.md
• tests/comfy_cli/command/test_run.py
• tests/comfy_cli/output/test_caller.py
• tests/comfy_cli/output/test_renderer.py
• tests/comfy_cli/test_tracking.py

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 5 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	2
🟢 Low	2

Panel: 8/8 reviewers contributed findings.

[mattmillerai]

Addressed the Cursor panel findings in 453d3e1 — 4 of 5 fixed, 1 deferred with reasoning. Re-triggered the panel to re-review the fix commit.

Finding	Disposition
🟠 Unguarded sys.stdout.isatty() at import	Fixed — guarded in detect_caller itself
🟡 caller_kind unbounded/unscrubbed	Fixed — scrub + 64-char cap at the telemetry boundary
🟡 partner_nodes[:20] does not cap element size	Fixed — each name capped at 64 chars
🟢 Uncapped node list in the error prose	Fixed — and N more suffix
🟢 Synchronous Mixpanel send on the hot path	Deferred to a follow-up — real, but the fix is architectural

Two notes worth a reviewer's attention:

The high-severity fix landed in caller.py, not tracking.py. The panel suggested guarding at the tracking call site. Guarding inside detect_caller instead also closes a pre-existing instance of the same bug: output/renderer.py:112 already called detect_caller(env_map) without is_tty, so the unguarded isatty() was reachable before this PR — it just was not at import time, so it degraded a single command rather than every command. One guard covers both.

I was wrong about the synchronous-send finding and changed my mind. I expected to rebut it on the grounds that track_command already puts a blocking send at the start of every command. It does not: run() (cmdline.py:762) carries no @tracking.track_command(), and partner_nodes_detected is the only track_event on the comfy run path. So this PR genuinely does introduce the first blocking telemetry round-trip there. It is Mixpanel-only (PostHog's capture queues to a background consumer) and bounded to one ~10s attempt by request_timeout=10, retry_limit=1.

I did not fix it here because the event has to precede the missing-credential rejection branch — that funnel is the metric BE-5633 exists to produce — and the real fix is making Mixpanel dispatch non-blocking, which has to be designed against the bounded atexit drain from BE-3403. Filed for the backlog and flagged as needing design sign-off rather than blind implementation. If you would rather not take that latency at all, the alternative is to hold this PR until the dispatch fix lands.

Verification: full suite green locally (3763 passed, 37 skipped); ruff check / ruff format clean on the touched files. New coverage — TestStdoutProbeIsFailSafe (None / closed / no-isatty stdout, plus env branches never probing stdout), TestSanitizeCallerKind, test_each_node_name_is_truncated_not_just_the_list, test_missing_credential_error_truncates_a_huge_node_list.

Still needs a human approval — I do not merge.

[mattmillerai]

🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:

• BE-5863 — Make Mixpanel telemetry dispatch non-blocking so events can't stall a command's hot path — filed as agent-spike (premise unverified)

The following carry agent-spike instead of agent-ok because their reachability claim was not backed by evidence (BE-5378) — the claim is investigated before any code is written, and "the premise does not hold" is a valid, successful outcome:

• Make Mixpanel telemetry dispatch non-blocking so events can't stall a command's hot path — no reachability block in the proposal

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 10 finding(s).

Severity	Count
🟠 High	2
🟡 Medium	5
🟢 Low	2
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[mattmillerai]

Addressed the second Cursor panel in a273852 — 6 of 10 fixed, 3 rebutted with reasoning, 1 already tracked. Each thread has the individual reasoning; the three things worth a reviewer's attention are below.

Finding	Disposition
🟠 Probe's except misses OSError/TypeError	Fixed — broadened to except Exception
🟠 Renderer.resolve still probes stdout unguarded	Fixed — shared probe, 4 call sites
🟡 Non-string class_type crashes the slice	Rebutted — filtered at parse time already
🟡 details payload still uncapped	Fixed — I was wrong last round
🟡 _scrub_value doesn't strip URL userinfo	Fixed (userinfo); rebutted (bare paths)
🟡 Hand-built dict bypasses filter_command_kwargs	Rebutted — routing it through is a no-op
🟡 caller_kind on the unconsented feedback path	Fixed — narrowed to the intrinsic kinds
🟢 Synchronous Mixpanel send on the hot path	Already tracked — BE-5863
🟢 Truncation after dedup reintroduces duplicates	Fixed — dedupe after truncation
⚪ Test claims a short-circuit it doesn't guard	Fixed — probe made lazy, test counts probes

The high-severity finding was right that last round's fix didn't actually fix anything. The panel's point was sharp: guarding detect_caller alone left Renderer.resolve calling sys.stdout.isatty() unguarded (renderer.py:114) from the main Typer callback with no is_stdout_tty (cmdline.py:250), so under pythonw/detached stdout comfy --help still died — moments later, and for the same reason. prompt_tracking_consent had the same bare probe on both stdin and stdout. And the tests I added exercised detect_caller in isolation, so they gave false confidence rather than catching it. There is now one caller.stream_is_tty, four call sites route through it, and the new tests drive Renderer.resolve and prompt_tracking_consent themselves — including a live-TTY case so the guard can't silently flip healthy terminals to JSON.

I reversed myself on the details cap. Last round I wrote a code comment arguing details should stay complete because it's the machine-readable field. That was wrong, and checking the panel's claim is what showed it: Renderer.error hands details to error_panel (renderer.py:307), which prints every entry as a key=value row directly beneath the message (panels.py:81-85). The full list was printing right under the message I had just truncated, so capping only the prose bounded nothing. details is now bounded identically, with partner_node_count carrying the exact total; docs/json-output.md records the contract change.

Three findings are rebutted rather than fixed, each because the premise doesn't hold rather than because the concern is unimportant — non-string class_type is already filtered at preflight.py:151-153; filter_command_kwargs on that payload passes all three stages untouched (_scrub_value guards on isinstance(value, str) and doesn't recurse); and stripping usernames out of bare filesystem paths isn't tractable, though I did fix the userinfo leak the same finding correctly identified and stopped the docstring overclaiming.

One thing I deliberately did not touch: the BE-4326 spend gate five lines above passes the same uncapped list in its own details. Pre-existing code this PR doesn't touch, and the exposure is a local terminal rendering the user's own workflow file. Flagged on the thread rather than widening the diff into another ticket's code.

Verification: full suite green (3783 passed, 37 skipped); ruff check and ruff format --diff clean under 0.15.15, the version CI pins. Worth noting for anyone running lint locally: uv run ruff resolves 0.12.7 from the lockfile, which still had the since-removed UP038 rule and reports 17 false positives in untouched files — use the CI pin.

Still needs a human approval — I do not merge.

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 8 finding(s).

Severity	Count
🟠 High	1
🟡 Medium	3
🟢 Low	3
⚪ Nit	1

Panel: 8/8 reviewers contributed findings.

[mattmillerai]

Third panel round, on the previous commit's own fixes — all 8 findings valid, all fixed in 9fb8218. Four were defects this branch introduced, so this round earned its keep.

Finding	Disposition
🟠 JSON mode only defers the crash to first emit	Fixed — but not the obvious way, see below
🟡 getattr lookup sits outside the try	Fixed — lookup moved inside
🟡 Spend gate still passes raw names	Fixed — bounded inside _spend_gate
🟡 Scrub ordering strands part of the credential	Fixed — a real leak I introduced
🟢 COMFY_USER_AGENT=user bypasses the narrowing	Fixed — keyed on source_env
🟢 _maybe_nudge_setup still bare sys.stderr.isatty()	Fixed — routed through the probe
🟢 Cap applied before dedup drops distinct names	Fixed — cap on distinct names
⚪ and N more conflates two reductions	Fixed — counts only cap-drops

The scrub finding was a genuine credential leak, not a hardening nit. My a273852 fix split the query and fragment off before the userinfo and bounded the authority at the first / — both of which assume a well-formed URL, when the values that worry us are precisely the malformed ones. https://svc:s3cr?et@host/agent shipped https://svc:s3cr, stranding half the password; https://svc:ab/cd@host/agent shipped verbatim with the password fully intact, and base64-ish tokens routinely contain /. Userinfo now comes off first, on the last @ in the remainder, so the credential's own contents can't move the cut point. Worth noting urlsplit would not have saved this — it parses that first example as query-then-path and returns the same wrong authority.

The high-severity fix is deliberately incomplete, and the repo's own test is why. The natural handler is (AttributeError, ValueError, OSError). I wrote that first, and test_json_login_fails_fast_when_login_url_write_breaks failed: comfy cloud login depends on BrokenPipeError propagating out of the login_url emit so it fails fast rather than blocking the full 300s on a browser callback nobody is left to read. A broken pipe means the stream was real and the reader hung up — a different situation from "there is no stream", and one this repo already decided should abort. So the guard covers AttributeError/ValueError (stdout None or closed) only, with OSError and TypeError still propagating, and tests now pin both directions so nobody widens it later.

I reversed myself on the spend gate, which I declined last round as pre-existing. That reasoning didn't survive my own docstring: I wrote "for every payload that leaves this process or reaches a terminal" into _bounded_node_names while leaving a branch that falsifies it — and the panel is right that it's the more commonly hit branch, since it fires before any credential resolution. Bounding now lives inside _spend_gate, so execute and execute_cloud are covered by construction.

Verification: full suite green (3793 passed, 37 skipped); ruff check and ruff format --diff clean under 0.15.15, the CI pin.

Still needs a human approval — I do not merge.