Repository navigation
fix(cli): a project .env cannot set the feedback email in another letter case - #5018
Conversation
Edit accuracy: accurate 1556 (base branch 1556), smooth 1364 of thoseThe gate passes. Quarantined, measured but not gated (0) |
terencecho
left a comment
There was a problem hiding this comment.
Approving 62e0c17b. A project .env can no longer set the feedback email under any spelling of the name, and hyperframes capture's .env loader now goes through the same applyDotEnv. I found nothing that blocks; the capture-side behaviour changes below are ones the PR body already lists.
It touches 4 files (dotEnv.ts, scaffolding.ts and their tests). Not draft, not stacked: 2 commits on main, same 4 files in main...62e0c17b. main is 4 commits ahead but none touch these files.
What I verified (head tarball, deps built, NODE_ENV=test)
dotEnv,feedbackSource,scaffolding,captureAttempttests: 21/21, 3 runs, matching the PR body.tsc --noEmitonpackages/cli,oxlintandoxfmt --checkclean.- Both new tests fail on base. Head
dotEnv.tswith the basescaffolding.tsfails the capture test; the basedotEnv.tsfails both. - 9 mutants: 8 caught (no
toUpperCase, wrong-case compare, skip dropped, overwrite of existing keys, capture applying to a scratch env, a one-level walk,exportprefix dropped, inline-comment stripping dropped). 1 survives, below. - Probe of what
applyDotEnvdoes with a feedback-email line: exact, lower, mixed case,export-prefixed, padded with spaces, after a BOM and with CRLF are all skipped, andOTHER=1beside them still loads. Only the key spelling is matched, so on Linux and macOS a lower-casehyperframes_feedback_emailis now dropped too; that is a distinct variable nothing reads, so it is harmless. - Only
commands/feedback.ts:281readsfeedbackEmail(), so the PR's statement that capture's old copy could not leak the email today holds.
Non-blocking
- Two capture-side changes can switch vision captioning off silently. Both are in the PR body, and both match what
cli.tsalready does for the cwd.env, but no test pins either. I ran the old inline loader againstapplyDotEnv:HYPERFRAMES_VERTEX_SERVICE_ACCOUNT="{"type":"service_account",…}"(double-quoted, inner quotes unescaped) used to load whole; it now loads as{, andcontentExtractor.ts:378then reports "not valid JSON; skipped vision captioning". Single-quoted and unquoted JSON load the same as before. dotenv-style parsers read the unescaped form whole, so this is the case most likely to surprise someone.- A shell variable already set to an empty string (
GEMINI_API_KEY=) is no longer filled from the.envabove the output folder. The old loader replaced empty values.
A line in the changelog or a pinning test for each would make it deliberate.
- "One owner" holds for
packages/cli. The media-use skill has its ownloadEnvFromDir(skills/media-use/audio/scripts/lib/heygen.mjs) that still copies every key, the feedback email in any case included. It runs in the skill scripts' own processes, which never callfeedbackEmail(), so it is the same non-leak as capture's old copy. Worth knowing if that script ever shells out tohyperframes feedback. - Survivor: capture's "first
.envfound wins"breakis not pinned (a walk that keeps loading parent.envfiles passes). That behaviour is unchanged by this PR. - Not exercised: native Windows. The tests check the rule directly, and the Windows case-blind
process.envbehaviour it guards is Node's documented one.
CI. All checks at this head finished: 76 passed, 5 skipped, 0 failing, 0 pending, including the edit-accuracy gate (1556, same as base). CI is a reference; the verdict rests on the evidence above.
Reviewed on the PR head 62e0c17b; no other review exists on it. This is a review verdict, not authorization to merge or deploy beyond what the gate already does.
— Review by tai (pr-review)
What
A project
.envcan no longer set the feedback email by spelling its name in another letter case.hyperframes_feedback_email=…(or any mixed case) in a project.envis now skipped, the same asHYPERFRAMES_FEEDBACK_EMAIL=…already was.hyperframes capture, which loads the.envabove its output folder, now applies the same rules instead of copying every key, the feedback email included.Why
Only the app that launches the CLI may attach an email to feedback; an agent can write a project
.env, soapplyDotEnvskips that key. The skip compared the name exactly. On Windows, environment variable names are case-blind, soprocess.env.hyperframes_feedback_email = xsets the same variablefeedbackEmail()reads, and feedback that should be anonymous would carry an email from the project file.Capture had its own
.envparser with no skip at all. Feedback is sent only by thefeedbackcommand, in its own process, so that copy could not leak the email today, but it was a second set of rules for the same file.How
applyDotEnv(packages/cli/src/utils/dotEnv.ts) compares the key upper-cased againstFEEDBACK_EMAIL_ENV.feedbackEmail()is unchanged.loadEnvFile(packages/cli/src/capture/scaffolding.ts) keeps its walk up to five parent folders and hands the first.envit finds toapplyDotEnv, which is now the one owner of how a project.envis applied..envhandling:export KEY=…lines and inline#comments are parsed, a quoted value ends at its closing quote (so a double-quoted JSON value with unescaped inner quotes is cut short, as it already was for the CLI's own.env), and a variable already set to an empty string is no longer overwritten by the file.Test plan
dotEnv.test.ts: "never takes the feedback email under another letter case" (lower and mixed case) fails on main (expected { …(3) } to deeply equal { OTHER: '1' }).scaffolding.test.ts: "loadEnvFile never takes the feedback email from a project file, in any letter case" writes a.envone folder above the start folder; it fails on main (expected [ 'a@example.com', …(2) ] to deeply equal [ undefined, undefined, 'kept' ]) and checks a quoted ordinary key is still loaded.dotEnv,feedbackSource,scaffoldingandcaptureAttempttests: 21 pass, 3 runs in a row (Linux). Pre-commit typecheck, oxlint, oxfmt and the comment ratchet pass.process.env.Size
Under the 100-line floor on purpose: a standalone privacy fix with nothing open to carry it.
No visible change
CLI only; nothing under Studio or the player.