Repository navigation
Conversation
applyDotEnv parsed .env content line by line: a quoted value's closing
quote was only ever looked for on the same physical line, so a value
like HYPERFRAMES_VERTEX_SERVICE_ACCOUNT='{...}' spread across several
lines got truncated to the leftover text on its opening line, and the
continuation lines were silently dropped (no "=" to parse as entries).
Join physical lines into logical ones before parsing: when a line
opens a quote that isn't closed on that same line, keep appending
following lines until one contains the closing quote character. This
matches how Node's util.parseEnv treats quoted multiline values.
Fixes heygen-com#6075
miguel-heygen
left a comment
There was a problem hiding this comment.
Thank you for picking this up, and the write up of the root cause is exactly right: valueOf only ever looked for the closing quote on one physical line. The join works, but it runs before anything classifies the line, so a commented out example swallows the rest of the file. With # OPENROUTER_API_KEY="sk-... on the line above a real OPENROUTER_API_KEY=..., the comment opens a quote that never closes, every later line is absorbed into it, and the parser returns no keys at all where main returned both. A stray unquoted quote anywhere does the same thing. Node's util.parseEnv keeps the later keys in both cases, so this also moves away from the parity the description claims.
Two small changes fix both: skip blank and # lines before looking for an open quote, and if no closing quote is found before the end of the file, fall back to the single physical line and carry on. Could you also pin a CRLF case? The joined value currently keeps its \r.
One more thing: the test suite has not run on this branch yet, only the mergeability and WIP checks, so the new test has not been exercised in CI.
jrusso1020
left a comment
There was a problem hiding this comment.
Requesting changes at 59cad9c, agreeing with Miguel's review at this same head. The root cause write-up is right and the single-quoted JSON case from #6075 now works, but the join runs on lines the parser would otherwise skip, and it has no way back when a quote never closes.
I ran applyDotEnv from this head directly:
# OPENROUTER_API_KEY="sk-oldfollowed byOPENROUTER_API_KEY=realandGEMINI_API_KEY=greturns{}. On main it returns both keys. A commented-out example is a common thing to find in a.env, so this is a regression for people who never had a multiline value.A='xfollowed byB=2givesA = "x\nB=2"and dropsB.- With CRLF line endings the joined value keeps its
\rcharacters ({\r\n "x": 1\r\n}), which is worth a test either way.
What would fix it: only look for an open quote on lines entryOf would actually parse (skip blank and # lines), and if the end of the file comes before a closing quote, push the opening line alone and continue from the next line, so behaviour matches main for anything that is not a real multiline value. A test for each of the three cases above would pin it.
Need: yes, #6075 is real on main · Size: ok · Reuse: nothing to reuse in the repo (Node's util.parseEnv is the reference behaviour) · Simpler: already small; the fix is two guards, not more machinery
— Rames
| const lines: string[] = []; | ||
| for (let i = 0; i < rawLines.length; i++) { | ||
| const line = rawLines[i] ?? ""; | ||
| const quote = openQuoteOf(line.trim()); |
There was a problem hiding this comment.
This runs before the blank/comment check in entryOf, so # KEY="sk-... opens a quote and swallows every line after it. I got {} for a file with a commented example above two real keys; main returns both. Skipping blank and # lines here fixes it.
| while (++i < rawLines.length) { | ||
| const next = rawLines[i] ?? ""; | ||
| continuation.push(next); | ||
| if (next.includes(quote)) break; |
There was a problem hiding this comment.
If no later line contains the quote, the loop runs to the end of the file and everything after the opening line becomes part of the value (A='x + B=2 gives A = "x\nB=2", no B). When the loop ends without a match, falling back to the opening line alone and resuming at the next line keeps main's behaviour for a stray quote.
… lines logicalLines looked for an open quote on every physical line before entryOf had a chance to classify it as blank or a comment, so a commented-out example like `# KEY="sk-old` (an unclosed quote inside a comment) opened a quote that never closed, joining every line below it into one value and dropping the real keys that followed. A genuinely stray unclosed quote with no later closing line had the same failure mode: the join ran to EOF and swallowed the rest of the file instead of leaving the malformed line on its own, so a later file like A='x / B=2 lost B entirely. Skip blank/comment lines before checking for an open quote (matching entryOf's own skip), and if the closing-quote search reaches EOF without finding one, fall back to the opening line on its own and resume parsing from the next line. Also strip a trailing \r per line so a CRLF multiline value doesn't carry it into the joined string.
|
@miguel-heygen @jrusso1020 thanks for catching this — you were both right, and it was a real correctness regression, not a nitpick. Pushed a fix in 666bf15. Root cause #1 (the one jrusso1020 verified by running it): Root cause #2: a genuinely stray/malformed unclosed quote with no closing line anywhere later in the file hit the same failure mode — the join ran to EOF and ate the rest of the file instead of leaving the malformed line on its own (e.g. Fix:
Verification: ran |
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround, all three cases from the last round are fixed. I ran applyDotEnv from this head against main and Node's util.parseEnv: a commented-out # KEY="sk-... above real keys keeps both keys, a stray unclosed quote no longer eats the rest of the file (A='x then B=2 gives both, as on main), and a CRLF multiline value comes back without \r. The three new tests fail on the previous head and pass here, so they pin the fixes.
One thing left before this can go in: the comment check will fail. node scripts/comment-ratchet.mjs reports dotEnv.ts at 13 comment lines out of 100 against 4 of 57 on main. Dropping the comments on openQuoteOf and closingLineIndex (the names already say it) and cutting the logicalLines block to two lines should clear it. While you are there, the PR description still describes only the first commit; a line on the comment skip, the end-of-file fallback and CRLF would help whoever reads it later.
The test workflows have not run on this branch yet (fork PRs need a maintainer to start them), so the new tests have not been through CI.
Verdict: COMMENT
Reasoning: All earlier correctness findings are fixed and verified, but the comment check fails locally and no required check has run at this head yet; switch to APPROVE once both are green.
…et budget Dropped the comments on openQuoteOf and closingLineIndex (their names already say what they do) and cut the logicalLines comment down to two lines, bringing the file back to 6/93 comment lines against the base's 4/57.
Root cause
applyDotEnv(packages/cli/src/utils/dotEnv.ts) parsed.envcontent by splitting on\nand handing each physical line toentryOf/valueOfindependently. For a quoted value,valueOfonly ever looks for the closing quote on that same line (raw.indexOf(quote, 1)); when the closing quote is on a later line, it falls back toraw.slice(1)— just the leftover text on the opening line — and every continuation line is dropped silently, since it has no=and failsentryOf'seq < 1check.This truncates values like:
down to
{instead of the full JSON blob, which later failsJSON.parse.Fix
Added
logicalLines, which joins physical lines into logical ones before parsing: when a line opens a quote ("or') that isn't closed on that same line, it keeps appending the following lines until one contains the closing quote character.entryOf/valueOfthen see the whole multiline value as a single string, andvalueOf's existingindexOf(quote, 1)naturally finds the real closing quote across the joined lines, preserving internal newlines. This matches how Node'sutil.parseEnvtreats quoted multiline values.The quote-detection logic was split into a small
openQuoteOfhelper to keeplogicalLineswithin the repo's complexity budget.Test
Added a regression test in
dotEnv.test.tsusing the exact repro from #6075: a quoted multilineHYPERFRAMES_VERTEX_SERVICE_ACCOUNTvalue followed by another key, asserting the full value round-trips throughJSON.parseand that the key after it still parses correctly.Verification
bunx vitest run src/utils/dotEnv.test.ts(inpackages/cli) — 7/7 passed, including the new testnode_modules/.bin/oxlinton changed files — 0 warnings/errorsnode_modules/.bin/oxfmt --checkon changed files — correctly formattedbun run --filter @hyperframes/cli typecheck— passesnode scripts/check-comment-citations.mjsandnode scripts/comment-ratchet.mjs— passUpdate: fixed a regression found in review
logicalLinescheckedopenQuoteOfon every physical line beforeentryOfcould classify it as blank or a#-comment. A commented-out example left above a real value (e.g.# OPENROUTER_API_KEY="sk-old) opens a quote that never closes, so the join ran to EOF and swallowed every real key below it. A genuinely stray unclosed quote with no later closing line hit the same failure mode, consuming the rest of the file instead of staying contained to its own line.Fixed by:
#-comments before ever checking for an open quote, mirroring the skipentryOfalready does.closingLineIndex; if it reaches EOF without finding one,logicalLinesnow falls back to the opening line on its own and resumes from the next line, instead of consuming everything after it.\rper line so a CRLF-authored.envdoesn't leak\rinto a joined multiline value.Added three more regression tests covering each case, verified by running
applyDotEnvdirectly against the exact repro shapes (not just the vitest assertions), and trimmed the added comments back down to stay within the repo's comment-ratchet budget.Fixes #6075