Code review findings: feat/ts-migration/scaffold

[luarss]

Code review of the feat/ts-migration/scaffold branch. 8 findings, ranked by severity.

---

1. parseBool silently disables security flags on misconfigured env vars

File: typescript/src/config/settings.ts:6

parseBool returns false for any string not in ["true", "1", "yes"] — including "enabled", "ENABLE", "TRUE", or a typo like "ture". Since this is used for OPENROAD_WHITELIST_ENABLED and OPENROAD_ENABLE_COMMAND_VALIDATION, a misconfigured env var silently disables security features with no error or warning.

Fix: throw on unrecognised values, or extend the allowlist to include common falsy spellings and throw on anything else.

---

2. annotate mode deletes bare \r instead of converting to \n

File: typescript/src/utils/ansi_decoder.ts:67

remove mode: .replace(/\r/g, "\n") — \r → newline
annotate mode: .replace(/\r/g, "") — \r → deleted

Input "10%\r20%\r30%" (progress-bar overwrite pattern) in annotate mode collapses to "10%20%30%"; in remove mode it correctly becomes "10%\n20%\n30%".

Fix: Use "\n" in the annotate mode replace too.

---

3. Non-CSI escape sequences leak through in annotate/preserve/decode modes

File: typescript/src/utils/ansi_decoder.ts:56

ESCAPE_PATTERN = /\x1b\[[0-9;?]*[a-zA-Z]/g only matches CSI sequences (ESC [). OSC sequences (\x1b]0;title\x07), charset designations (\x1b(B), and single-char escapes (\x1bM) are not matched. In remove mode, stripAnsi strips all of them. In every other mode, they pass through as raw bytes.

Fix: Either extend ESCAPE_PATTERN to cover other sequence families, or run a stripAnsi pass in non-remove modes to sanitise residual sequences.

---

4. Module-level Settings.fromEnv() throws synchronously if any numeric env var is invalid

File: typescript/src/config/settings.ts:133

export const settings = Settings.fromEnv() fires at import time. If OPENROAD_COMMAND_TIMEOUT=notanumber (or any numeric field is set to a bad value), the process crashes at startup with a raw Error thrown from inside module initialisation — no catch, no context.

Fix: Wrap in a lazy getter or an explicit initSettings() call in main.ts, so startup errors are caught and reported with useful context.

---

5. Mode validation bypassed for empty input

File: typescript/src/utils/ansi_decoder.ts:86

if (!text) return text runs before the throw new Error("Unknown mode: ...") at the end. translateOutput("", "bogus") returns "" silently.

Fix: Move mode validation before the early return, or validate mode as a union type so TypeScript rejects invalid modes at compile time.

---

6. Unescaped ? in two ESCAPE_SEQUENCES patterns

File: typescript/src/utils/ansi_decoder.ts:5–6

"\\x1b\\[?\\d*h": "Enable terminal mode",
"\\x1b\\[?\\d*l": "Disable terminal mode",

The ? is a regex quantifier (making [ optional), not a literal ?. As a result, \x1bh (VT100 "Move to Next Tab") and \x1bl are matched and misclassified as terminal mode sequences.

Fix: Escape the ? if the intent is to match private-mode sequences, or clarify these as standard-mode patterns and document the distinction.

---

7. Dead fallback code for ?2004h/?2004l

File: typescript/src/utils/ansi_decoder.ts:64–65

Lines 64–65 (if (sequence.includes("?2004h"))...) are unreachable — the ESCAPE_SEQUENCES dict already has exact entries for these sequences that always match first in the loop above.

Fix: Remove lines 64–65.

---

8. new RegExp() compiled inside loop on every decodeEscapeSequence call

File: typescript/src/utils/ansi_decoder.ts:61

~45 new RegExp() objects are created per decodeEscapeSequence call with no caching. On a session transcript with many unique sequences this creates significant GC pressure.

Fix: Pre-compile the patterns once at module load:

const COMPILED_SEQUENCES: Array<[RegExp, string]> = Object.entries(ESCAPE_SEQUENCES)
  .map(([pat, desc]) => [new RegExp(`^${pat}`), desc]);