test(cli): real-render e2e check for render telemetry properties - #4018

Reviewed at 71bc098, base is #4017's branch not main. Own-account, comment-only.

This is the check Home asked for by name: does the test fail if the properties were wrong, not just if they're absent. It does. cli-telemetry-e2e.mjs uses a paired positive/negative fixture: the positive fixture (media present, root/body CSS size mismatched by more than 50px, --skill flag, an injected HYPERFRAMES_E2E_MARKER env var) asserts audio_count === 1, image_count === 1, root_body_mismatch === true, root_body_delta_px_bucket === "51+", authoring_skill_source === "flag", and that hf_env_overrides includes the injected name. The negative fixture (no media, matched size, no skill flag, clean env) asserts the opposite exact values: === 0, === false, === "0", authoring_skill_source absent, hf_env_overrides empty. A stub or an always-present/always-absent implementation fails one side or the other; this is a real discriminating pair, not a presence check.

One honest gap, not a blocker: because this PR's base is #4017's branch and the repo's ci.yml only triggers pull_request runs against base: main (by design, per the workflow's own Graphite-stack comment), the full smoke suite including CLI smoke (required) (the job that actually runs this script) has not executed on either #4017's or #4018's head yet, since #4017 doesn't touch the script and #4018 is stacked. It will run once this restacks onto main. Read the script itself carefully in place of a CI run; logic is sound.

Verdict: clean, no blockers
Reasoning: the value-discriminating test design directly answers the exact failure mode this check was written to catch; the CI-hasn't-run-yet gap is structural to stacking, not this PR's fault, and Home already flagged it as expected.

:large_green_circle: 71bc0986 LGTM

Clean extension of the existing built-CLI harness. Same fetch-wrapping hook, same batch-capture shape, same "no PostHog request escapes" invariant — just plumbing a second command through it. The positive/negative fixture pair asserts every render-telemetry property that #4017 introduced, and the pair discriminates on each (mismatch true vs false, delta bucket 51+ vs 0, image/audio counts 1 vs 0, skill source flag vs absent, one env override vs empty array). One small hygiene gap on the negative side worth noting; not a blocker.

What the change is, verified end-to-end
• Extends scripts/ci/cli-telemetry-e2e.mjs (already wired at .github/workflows/ci.yml:803, and the job already installs ffmpeg + exercises puppeteer via the preceding smoke render — no new CI dependency).
• Fixtures are generated at test time: PIXEL_PNG_BASE64 decoded into pixel.png, and spawnSync("ffmpeg", ..., "sine=frequency=440:sample_rate=48000", "-t", "1", ...) for tone.wav. Nothing checked in, no LFS. Grepped the diff — no new asset files.
• withoutHfEnv(process.env) uses /^(HF|HYPERFRAMES)_/, which matches production HF_ENV_OVERRIDE_RE in packages/cli/src/commands/render/plan.ts verbatim — so the harness's own inherited env can't leak into hf_env_overrides, and injected HYPERFRAMES_E2E_MARKER reliably surfaces. The PR body's story about this catching the bootstrap-wiring bug is consistent with CLI_INTERNAL_HF_ENV_KEYS.
• --skill "e2e-smoke-test" passes SKILL_SLUG (/^[a-z0-9][a-z0-9-]{0,63}$/) in packages/cli/src/telemetry/skill.ts, so it drives authoringSkillSource="flag" end-to-end via plan.ts → render.ts → events.ts:445. The assertion is real.
• authoring_skill_source on the negative side deliberately uses !("... in ...) rather than === undefined — correct, because it guards against the regression where the field lands as a literal null (which would still evaluate as absent under === undefined on JSON parse). Comment in the harness even calls out "not a falsy placeholder". Good.
• runRender uses async spawn (not spawnSync), which is required — the fixture registry is served from this same process. Comment already covers this for runAdd; the render path is consistent.

Concerns
• (non-blocker) Negative side is missing the check("exactly one render_complete event", neg.completeEvents.length === 1, ...) sanity check that the positive side has. If the neg fixture ever emits zero render_complete events, negEvent = {} and !("authoring_skill_source" in {}) passes vacuously. The other neg checks (=== 0, === false, === "0") would still fail on undefined, so the harness overall would go red — the vacuous pass hides in a run that's already failing, not one that's silently green. But cheap to close: add the count assertion right after const negEvent = .... If it fires twice with divergent properties, [0] also silently ignores the second — same fix covers it.

Nits
• stdio: "ignore" on the render children means a genuine pipeline failure in CI produces no browser/ffmpeg stderr in the job log — you'd have to re-run locally to see it. Matches the existing runAdd pattern so consistent, but on a render (which has substantially more failure surface than add) piping stderr and dumping it only on non-zero would pay for itself. Not important.
• writeFixtureMedia's throw on ffmpeg failure skips server.close() and the rmSync(sandbox, ...) cleanup, so a bad run leaks the temp dir + listening port. Node's unhandled-rejection default kills the process, so no test hangs — just tidiness. A try/finally around the pos/neg block would cover it.

What I didn't verify
• Didn't run the harness locally against a built CLI — CI status rollup shows all 13 checks green on 71bc0986 and the PR body reports 24/24 (18 pre-existing + 6 new).
• Runtime cost of two --quality draft --workers 1 renders inside the 25-min job budget — the preceding smoke render in the same job establishes the browser is warm and the fixtures are one-frame 800x450, so should be fast, but I didn't wall-clock it.

— Review by Rames D Jusso

:large_green_circle: 71bc0986 LGTM (re-stamp after base retarget)

Prior APPROVE 5230226196 was auto-dismissed when the base retargeted from feat/render-telemetry-output-and-environment to main. HEAD SHA is unchanged, no new commits since my prior review (ca ee f0 90 of 71bc0986 verified). #4017 squash-merged into main at 02:10:28Z, so once this rebases onto main the visible 16-file / 1569-add surface collapses back to the single 196-line test-file extension I reviewed. All CI green end-to-end at HEAD (44 checks success, 14 skipped, zero failures).

Prior verification carried forward verbatim:

Clean extension of the existing built-CLI harness. Same fetch-wrapping hook, same batch-capture shape, same "no PostHog request escapes" invariant — just plumbing a second command through it. The positive/negative fixture pair asserts every render-telemetry property that #4017 introduced, and the pair discriminates on each (mismatch true vs false, delta bucket 51+ vs 0, image/audio counts 1 vs 0, skill source flag vs absent, one env override vs empty array). One small hygiene gap on the negative side worth noting; not a blocker.

What the change is, verified end-to-end
• Extends scripts/ci/cli-telemetry-e2e.mjs (already wired at .github/workflows/ci.yml:803, and the job already installs ffmpeg + exercises puppeteer via the preceding smoke render — no new CI dependency).
• Fixtures are generated at test time: PIXEL_PNG_BASE64 decoded into pixel.png, and spawnSync("ffmpeg", ..., "sine=frequency=440:sample_rate=48000", "-t", "1", ...) for tone.wav. Nothing checked in, no LFS. Grepped the diff — no new asset files.
• withoutHfEnv(process.env) uses /^(HF|HYPERFRAMES)_/, which matches production HF_ENV_OVERRIDE_RE in packages/cli/src/commands/render/plan.ts verbatim — so the harness's own inherited env can't leak into hf_env_overrides, and injected HYPERFRAMES_E2E_MARKER reliably surfaces. The PR body's story about this catching the bootstrap-wiring bug is consistent with CLI_INTERNAL_HF_ENV_KEYS.
• --skill "e2e-smoke-test" passes SKILL_SLUG (/^[a-z0-9][a-z0-9-]{0,63}$/) in packages/cli/src/telemetry/skill.ts, so it drives authoringSkillSource="flag" end-to-end via plan.ts → render.ts → events.ts:445. The assertion is real.
• authoring_skill_source on the negative side deliberately uses !("... in ...) rather than === undefined — correct, because it guards against the regression where the field lands as a literal null (which would still evaluate as absent under === undefined on JSON parse). Comment in the harness even calls out "not a falsy placeholder". Good.
• runRender uses async spawn (not spawnSync), which is required — the fixture registry is served from this same process. Comment already covers this for runAdd; the render path is consistent.

Concerns
• (non-blocker) Negative side is missing the check("exactly one render_complete event", neg.completeEvents.length === 1, ...) sanity check that the positive side has. If the neg fixture ever emits zero render_complete events, negEvent = {} and !("authoring_skill_source" in {}) passes vacuously. The other neg checks (=== 0, === false, === "0") would still fail on undefined, so the harness overall would go red — the vacuous pass hides in a run that's already failing, not one that's silently green. But cheap to close: add the count assertion right after const negEvent = .... If it fires twice with divergent properties, [0] also silently ignores the second — same fix covers it.

Nits
• stdio: "ignore" on the render children means a genuine pipeline failure in CI produces no browser/ffmpeg stderr in the job log — you'd have to re-run locally to see it. Matches the existing runAdd pattern so consistent, but on a render (which has substantially more failure surface than add) piping stderr and dumping it only on non-zero would pay for itself. Not important.
• writeFixtureMedia's throw on ffmpeg failure skips server.close() and the rmSync(sandbox, ...) cleanup, so a bad run leaks the temp dir + listening port. Node's unhandled-rejection default kills the process, so no test hangs — just tidiness. A try/finally around the pos/neg block would cover it.

What I didn't verify
• Didn't run the harness locally against a built CLI — CI status rollup shows 44 checks green + 14 skipped on 71bc0986 and the PR body reports 24/24 (18 pre-existing + 6 new).
• Runtime cost of two --quality draft --workers 1 renders inside the 25-min job budget — the preceding smoke render in the same job establishes the browser is warm and the fixtures are one-frame 800x450, so should be fast, but I didn't wall-clock it.

HF-OSS require_last_push_approval=true — any push voids this stamp.

— Review by Rames D Jusso