# CODE_QUALITY_AUDIT.md — RobCo U.O.S. v2.6.0 > **FILED UNDER 2.8.0 — PRODUCED-BY, NOT MEASURED-VERSION.** This document *measures* the earlier v2.6.0/v2.7.0 > codebase, but it was *produced* as a stage of the PARKED-CHAIN audit sequence — the analysis phase that became > the 2.8.0 overhaul. Planning artifacts are filed by the release whose work produced them, never by the version > they discuss. The version in the title is the code it audited, not the folder it belongs in. > **[ARCHIVE-class snapshot — not current truth.]** Per the 2.8.5 U-B1 doc-maintenance model (see `CLAUDE.md`'s > Reference Pointer Index), this doc is frozen as of the date below and is never updated to track current code. It > audited a codebase roughly 30% smaller than today's (pre-`test-console.js`, pre-OCR, pre-Diagnostic-Shell, > pre-CHASSIS/Overseer). Reuse its structural reasoning as historical input only — re-verify every measurement and > file-split proposal against current code before acting on it. > Stage A5 of the PARKED-CHAIN audit sequence. PLAN-ONLY. Nothing here is implemented or committed. > Companion docs: HOUSE_STANDARD.md (A0), DIEGETIC_AUDIT.md (A0), PERFORMANCE_AUDIT.md (A1), > ACCESSIBILITY_AUDIT.md (A2), CONTENT_AUDIT.md (A3), MOBILE_AUDIT.md (A4). > Target: bc8c2eb (v2.6.0-r1, 1078 tests). > > Codebase stats: 10 source JS files, 10,993 lines total. > Key file sizes: ui-render.js 1,943 · ui-core.js 1,925 · api.js 1,423 · reg_nv.js 1,942 · reg_fo3.js 1,087 --- ## Summary counts | Severity | Count | |---|---| | **HIGH** | 6 | | **MEDIUM** | 10 | | **LOW** | 16 | | **SPEC-FIRST** | 4 | | **Total actionable** | **36** | --- ## Section 1 — PROHIBITED PATTERN VIOLATIONS These are the items from the RULES.md Prohibited Patterns table. Any that are confirmed violations require a fix pass; they are not style choices. --- ### QA-PROHIB-1 · [HIGH] · `innerHTML +=` inside `.forEach` loop — api.js:257 **Confirmed violation of the prohibited pattern.** ```js data.models.forEach(m => { // ... filter ... selectEl.innerHTML += `<option value="${shortName}">${m.displayName || shortName} (${shortName})</option>`; added++; }); ``` Each iteration re-serializes and re-parses the entire `<select>` DOM subtree. At N models, this is O(n²) DOM work — the exact pattern the RULES.md table bans. **Fix:** Collect `shortName` values in an array, then assign once after the loop: ```js const opts = []; data.models.forEach(m => { if (...) opts.push(`<option value="${sh}">${sh}</option>`); }); selectEl.innerHTML = opts.join(''); ``` **Risk:** Behavior-neutral (same options, same order). No test coverage currently guards this specific function. **Verdict:** BUILD-NOW · `[CACHE][TEST]` (add a static guard that `api.js` contains no `innerHTML +=`) **Files:** `js/api.js` --- ### QA-PROHIB-2 · [HIGH] · Unescaped `localStorage` value injected into `innerHTML` — ui-core.js:158 ```js let savedModel = localStorage.getItem('robco_gemini_model'); document.getElementById('apiModelInput').innerHTML = `<option value="${savedModel}">${savedModel} (Secured)</option>`; ``` `savedModel` is injected into both the `value` attribute and the text content without `escapeHtml()`. A crafted value in `localStorage` (set via DevTools, a future XSS, or a localStorage poisoning attack) can inject arbitrary HTML or script content into the model dropdown. The existing XSS suite (Suites 22–41) covers other injection points; this specific one is ungated. **Fix:** ```js const s = escapeHtml(savedModel); document.getElementById('apiModelInput').innerHTML = `<option value="${s}">${s} (Secured)</option>`; ``` **Risk:** Behavior-neutral. The escaped and unescaped values are identical for all well-formed model names. **Verdict:** BUILD-NOW · `[CACHE][TEST]` (extend XSS suite to cover apiModelInput) **Files:** `js/ui-core.js` --- ### QA-PROHIB-3 · [MEDIUM] · Unescaped Firestore data injected into `innerHTML` — cloud.js:586 ```js modelInput.innerHTML = `<option value="${data.model}">${data.model} (Secured)</option>`; ``` `data.model` arrives from Firestore (user-written, but external untrusted storage). Not `escapeHtml()`-wrapped. Lower risk than PROHIB-2 (Firestore data is written by the same authenticated user), but the pattern is inconsistent with the codebase's XSS discipline. **Fix:** Same as PROHIB-2 — wrap in `escapeHtml()`. **Verdict:** BUILD-NOW · `[CACHE][TEST]` · **Severity: MEDIUM** **Files:** `js/cloud.js` --- ### QA-PROHIB-4 · [MEDIUM] · `localStorage.getItem()` in `getSystemDirective()` — runs on every AI call **api.js:4:** ```js let playstyle = localStorage.getItem('robco_playstyle') || 'any'; ``` `getSystemDirective()` is called inside `transmitMessage()` on every AI invocation. `localStorage.getItem()` is a synchronous storage read; while it is not an audio hot path, it is the exact "hot path" class the rule targets — a function called frequently where the result could be cached from state. The `playstyle` value is stored in `localStorage` but not mirrored in `state.*`. The read here is the canonical source. **Fix:** Mirror `robco_playstyle` into `state.playstyle` (it already exists in the export envelope at state.js:483) and read `state.playstyle` here instead of `localStorage`. Or hoist the read one level up into `transmitMessage()` and pass it into `getSystemDirective()` as a parameter. **Verdict:** BUILD-NOW (playstyle already in save envelope — the read should come from state) · `[CACHE][TEST]` · **Severity: MEDIUM** **Files:** `js/api.js`, `js/state.js` --- ### QA-PROHIB-5 · [MEDIUM] · `localStorage.getItem()` in `transmitMessage()` hot path — api.js:1080–1081 ```js let rawKey = localStorage.getItem('robco_gemini_key'); let selectedModel = localStorage.getItem('robco_gemini_model'); ``` Called on every AI message. Both values are only written by `saveApiKeySilent()`. They should be cached in a module-level variable, invalidated when `saveApiKeySilent()` runs. **Fix:** Add module-level `let _cachedKey = null, _cachedModel = null;`. Set them in `saveApiKeySilent()` after the localStorage write. Read `_cachedKey`/`_cachedModel` in `transmitMessage()`, falling back to `localStorage.getItem()` on first call. **Verdict:** BUILD-NOW · `[CACHE][TEST]` · **Severity: MEDIUM** **Files:** `js/api.js` --- ### QA-PROHIB-6 · [MEDIUM] · Direct `state.*` access in `cloud.js` — boundary violation **cloud.js:260–263:** ```js const activeContext = state.gameContext || 'FNV'; const activeVersion = window.APP_VERSION || '2.6.0'; ``` `cloud.js` is the sync/IO layer and must not reach into `state.*` directly (Protocol 23: systems communicate only through established functions/interfaces). The `state` global is owned by `state.js`. The `gameContext` read bypasses the localStorage persistence pipeline. It happens to be correct because `state.gameContext` is always hydrated before cloud save is called, but it creates a hidden dependency ordering assumption. **Fix:** Read `gameContext` from `localStorage.getItem('robco_v8')` (the already-serialized state) or expose a thin getter: `window.getGameContext = () => state.gameContext` in state.js, called here. **Verdict:** SPEC-FIRST (requires state.js API change) · **Severity: MEDIUM** **Files:** `js/cloud.js`, `js/state.js` --- ## Section 2 — DEAD CODE --- ### QA-DEAD-1 · [MEDIUM] · `showDeltaGhost()` defined in ui-audio.js — zero call sites `showDeltaGhost(fieldId, oldVal, newVal)` is defined but never called anywhere in the codebase. It is not attached to `window`. It appears to have been written speculatively for a "delta ghost" overlay effect that was never wired up. **Fix:** Remove the function. If delta ghosting is desired in future, it can be added from git history. **Verdict:** BUILD-NOW · `[CACHE][TEST]` (add static guard that `showDeltaGhost` is absent) **Files:** `js/ui-audio.js` --- ### QA-DEAD-2 · [LOW] · `_inputHistory` declared but never written — ui-core.js:575 ```js const _inputHistory = []; // cycle through sent user commands ``` The arrow-key handler that follows reads from `chatHistory` (filtered to `sender === 'user'`), not from `_inputHistory`. The array is allocated and immediately orphaned. **Fix:** Delete line 575. **Verdict:** BUILD-NOW (one-line deletion) · `[CACHE]` **Files:** `js/ui-core.js` --- ### QA-DEAD-3 · [LOW] · `_cleanKey` assigned but never used — api.js:209 ```js const _cleanKey = encodeURIComponent(rawKey); ``` `rawKey` is used directly in the subsequent `fetch()` call. `_cleanKey` is computed and immediately discarded. Note: this may be a leftover from a version where the key was sent as a URL query parameter (which would require encoding) rather than a header (which doesn't). **Fix:** Delete line 209. **Verdict:** BUILD-NOW (one-line deletion) · `[CACHE]` **Files:** `js/api.js` --- ### QA-DEAD-4 · [LOW] · `ticksToGameTime()` in ui-render.js:260 — zero call sites in JS The function has a comment saying "kept for backward compatibility (log exports, any future consumers)." No JS file calls it. It IS listed as a `readonly` global in `eslint.config.mjs`, suggesting intentional retention. If the intent is to keep it for external consumers (e.g. future dev tools), the comment should say so explicitly. If it genuinely has no future use, it should be removed. **Fix:** Either add a clear comment: `/* public API: available for external tools, do not remove */` or delete the function and its eslint global entry. **Verdict:** SPEC-FIRST (owner decision needed — intentional keep vs removal) · **Severity: LOW** **Files:** `js/ui-render.js`, `eslint.config.mjs` --- ### QA-DEAD-5 · [LOW] · `gameTimeToTicks()` in ui-render.js:357 — zero call sites, wrong comment Comment says "used by time input handler" but the actual handler calls `calendarToTicks()`. `gameTimeToTicks` has zero call sites. **Fix:** Delete the function and update any ESLint global entry. **Verdict:** BUILD-NOW · `[CACHE]` **Files:** `js/ui-render.js` --- ### QA-DEAD-6 · [LOW] · `targetDT = 0` constant in `showVATSOverlay()` — always inert ```js const targetDT = 0; // Default: no DT. AI will handle specifics. ``` The constant feeds into the base-chance formula but is permanently zero. The formula is live code but the `targetDT` variable adds a named binding for a value that will never change. **Fix:** Inline `0` into the formula and remove the declaration. **Verdict:** BUILD-NOW (cosmetic only) · `[CACHE]` **Files:** `js/ui-core.js` --- ### QA-DEAD-7 · [LOW] · `#updateBanner` CSS in terminal.css — no matching HTML element The `#updateBanner` block (including `@keyframes update-banner-pulse`, ~35 lines) references an element replaced by `#updateModal` (Suite 65). No HTML, no JS reference. Pure dead CSS weight. **Cross-ref:** A4 M-M4 flagged this; included here for the CSS quality sweep. **Fix:** Delete the `#updateBanner` rule and `@keyframes update-banner-pulse` from terminal.css. **Verdict:** BUILD-NOW · `[CACHE][TEST]` (verify no Suite test guards the banner CSS) **Files:** `css/terminal.css` --- ### QA-DEAD-8 · [LOW] · Commented-out `gameContext` assignment — api.js:739 ```js // if (gcV === 'FNV' || gcV === 'FO3') state.gameContext = gcV; ``` Deliberately suppressed (security guard against AI overriding game context). The `_gcV` parse on line 738 runs but the value is unused. The intent is clear from context, but a future reader may enable the line thinking it's accidentally commented out. **Fix:** Delete both lines 738–739 and add a comment at the top of the `gameContext` block: `/* gameContext is not AI-settable — game context changes are user-only via onGameContextChange() */`. **Verdict:** BUILD-NOW (clarity, no behavior change) · `[CACHE]` **Files:** `js/api.js` --- ## Section 3 — DUPLICATION --- ### QA-DUP-1 · [HIGH] · `renderSkillBooks()` and `renderMagazines()` are ~90% identical The two functions (ui-render.js:903–965 and 979–1050) share the same structure line-for-line: | Shared element | `renderSkillBooks` | `renderMagazines` | |---|---|---| | State array split | `skill_books_read/unread` keys | `magazines_read/unread` keys | | `localStorage.getItem('robco_panel_state')` | ✓ | ✓ | | `rowHtml(d, isRead)` inner factory | ✓ | ✓ | | `<details class="sub-panel" data-sub-id="…">` | ✓ | ✓ | | `<summary><h3>> READ/UNREAD [N]</h3>` | ✓ | ✓ | | `toggle` event listener for panel state | ✓ | ✓ | | `emptyState()` calls | ✓ | ✓ | Differences: toggle function name, state key names, one skill-display column variation, empty-state strings. Every time a new read-tracker is added (e.g. comic books, holotapes), another ~130-line copy-paste would be created. This is the highest-value dedup opportunity in the codebase. **Fix:** Extract `_renderReadTracker(opts)` where `opts = { containerId, stateKey, defs, subIdRead, subIdUnread, toggleFn, emptyRead, emptyUnread, rowExtra? }`. Both `renderSkillBooks()` and `renderMagazines()` become ~10-line call-site wrappers. **Verdict:** SPEC-FIRST (non-trivial refactor, new helper function required) · **Severity: HIGH** **Files:** `js/ui-render.js` --- ### QA-DUP-2 · [MEDIUM] · `listLocalSaves()` in ui-account.js duplicates slot-reading from ui-saves.js `listLocalSaves()` (ui-account.js:3–29) reads `robco_v8` and `robco_slot_1/2/3` directly from `localStorage`. The canonical slot-reading and slot-schema logic lives in `ui-saves.js`. Any change to the slot schema (key names, count) requires updating both files. **Fix:** Move `listLocalSaves()` to `ui-saves.js` and expose via `window.listLocalSaves`. `ui-account.js` calls `window.listLocalSaves()`. **Verdict:** BUILD-NOW · `[CACHE][TEST]` (update any test that guards `listLocalSaves` location) **Files:** `js/ui-account.js`, `js/ui-saves.js` --- ### QA-DUP-3 · [MEDIUM] · Memory-cycle interval code copy-pasted in ui-core.js The `setInterval` block that appends `MEMORY CYCLE COMPLETE` + brightness flash appears verbatim at two sites: - ui-core.js:566–572 (initial boot path) - ui-core.js:241–248 (inside `exitStandby()`) Both are guarded by `if (!_memCycleInterval)` so there is no doubling bug, but the literal string `'> MEMORY CYCLE COMPLETE'` and the `0.35` brightness value are copy-pasted. A wording change needs editing in two places. **Fix:** Extract `function _startMemCycle()` called by both paths. **Verdict:** BUILD-NOW · `[CACHE]` **Files:** `js/ui-core.js` --- ### QA-DUP-4 · [LOW] · Uptime clock interval copy-pasted — same pattern as DUP-3 `setInterval` for `#uptimeClock` appears identically at ui-core.js:231–239 (in `exitStandby()`) and 554–562 (boot path). Same guard, same interval value. **Fix:** Extract `function _startUptimeClock()`. **Verdict:** BUILD-NOW · `[CACHE]` **Files:** `js/ui-core.js` --- ### QA-DUP-5 · [LOW] · `MONTH_DAYS` declared twice in ui-render.js ```js // line 275 inside _resolveGameDateTime(): const MONTH_DAYS = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]; // line 366 inside calendarToTicks(): const MONTH_DAYS = [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]; ``` Identical array, declared twice. A February change (e.g. leap year handling) would need updating in both. **Fix:** Hoist to `const _MONTH_DAYS = [...]` at file scope; both functions reference it. **Verdict:** BUILD-NOW · `[CACHE]` **Files:** `js/ui-render.js` --- ### QA-DUP-6 · [LOW] · 14+ identical `try/catch console.warn` blocks in cloud.js Every async cloud operation follows the pattern: ```js try { // ... operation ... } catch (e) { console.warn('operationName failed (non-fatal):', e); } ``` These are correct and intentional (non-fatal network failures), but the repetition inflates the file by ~100 lines. A thin helper `async function _cloudOp(fn, label)` would collapse these. **Verdict:** SPEC-FIRST (low risk but reshapes cloud.js structure significantly) · **Severity: LOW** **Files:** `js/cloud.js` --- ### QA-DUP-7 · [LOW] · `FALLOUT_REGISTRY.recipes/breakdowns` guard repeated 3× in ui-render.js The guard: ```js typeof FALLOUT_REGISTRY !== 'undefined' && Array.isArray(FALLOUT_REGISTRY.recipes) ? FALLOUT_REGISTRY.recipes : [] ``` appears 3 times for `recipes` and 3 times for `breakdowns`. A one-liner `function _reg(key) { return (typeof FALLOUT_REGISTRY !== 'undefined' && Array.isArray(FALLOUT_REGISTRY[key])) ? FALLOUT_REGISTRY[key] : []; }` already exists as a pattern for other keys — apply it here. **Verdict:** BUILD-NOW · `[CACHE]` **Files:** `js/ui-render.js` --- ### QA-DUP-8 · [LOW] · Audio guard style inconsistency — mixed `||` vs. two-line form Older audio functions use `if (AudioSettings.masterMute || AudioSettings.key) return;`. Newer functions use two separate `if` lines per Protocol 7. Both are functionally correct. No bug. **Verdict:** SKIP (cosmetic; fix when functions are touched for other reasons) --- ## Section 4 — CONSISTENCY --- ### QA-CONS-1 · [HIGH] · Faction F+/F− label displays ±50 — actual increment is ±5 **ui-render.js:1544:** ```js 'F+/F- = Fame ±50 &nbsp; I+/I- = Infamy ±50' ``` The buttons call `adjustFaction('...', 'fame', 5)` and `adjustFaction('...', 'infamy', 5)`. The label is 10× wrong. Users who read the tooltip and then observe a +5 change will think the system is broken. **Fix:** Change `±50` to `±5` in the label string (two occurrences on this line). **Verdict:** BUILD-NOW · `[CACHE][TEST]` (add a static guard that `±50` is absent from faction render) **Files:** `js/ui-render.js` --- ### QA-CONS-2 · [MEDIUM] · `window.onload` is ~540 lines — largest single block in the codebase `window.onload` (ui-core.js:75–613) encompasses: state hydration from v8/v7/fresh, faction migration, all mute-toggle restores, standby enter/exit logic + event listeners, panel-persistence wiring, sub-panel wiring, optics/speed/context/RNG restore, keyboard shortcut wiring, input history wiring, boot sequence + session briefing (~60 lines), uptime clock + memory cycle intervals, and the `beforeunload` handler. This function is the single hardest block to read and the most likely place for regression when any feature is modified. **Fix (decomposition candidates, each extractable independently):** - `_hydrateStateFromStorage()` — v8/v7/fresh hydration - `_restoreMuteToggles()` — the 12 mute-toggle `if` blocks - `_wirePanelPersistence()` — panel + sub-panel toggle event listeners - `_wireKeyboardShortcuts()` — keyboard event listeners - `_startSessionTimers()` — uptime + memory cycle intervals **Verdict:** SPEC-FIRST (decomposition is safe but touches every path in the app's boot; requires careful regression testing at every breakpoint) · **Severity: MEDIUM** **Files:** `js/ui-core.js` --- ### QA-CONS-3 · [LOW] · `loadUI()` double-populates skill inputs `loadUI()` calls `renderSkills()` at line 1339 (which sets all skill input values from `state.skills`), then iterates `getSkillKeys()` again at lines 1373–1376 to set the same input values a second time. The second pass is redundant. **Fix:** Remove the redundant skill-input loop (lines 1373–1376). `renderSkills()` is the canonical setter. **Verdict:** BUILD-NOW · `[CACHE][TEST]` **Files:** `js/ui-core.js` --- ### QA-CONS-4 · [LOW] · Hardcoded version fallback `'2.6.0'` in cloud.js:289 and 430 ```js version: window.APP_VERSION || '2.6.0', ``` If `APP_VERSION` is somehow undefined, cloud saves are labeled `'2.6.0'` indefinitely regardless of future versions. The fallback should be `'unknown'` to make the missing context explicit rather than silently wrong. **Fix:** Change `|| '2.6.0'` to `|| 'unknown'` at both sites. **Verdict:** BUILD-NOW · `[CACHE]` **Files:** `js/cloud.js` --- ### QA-CONS-5 · [LOW] · `saveApiKeySilent()` re-reads localStorage for a value just written ```js localStorage.setItem('robco_gemini_model', model); // line 277 // ... window.saveGeminiKeyToCloud(key, localStorage.getItem('robco_gemini_model') || ''); // line 279 ``` The `model` variable is already in scope — pass it directly. **Fix:** Replace `localStorage.getItem('robco_gemini_model') || ''` with `model || ''`. **Verdict:** BUILD-NOW · `[CACHE]` **Files:** `js/api.js` --- ### QA-CONS-6 · [LOW] · Magic number `-200` in ui-saves.js:103 should be `-CHAT_MAX` ```js const _slotChat = chatHistory.slice(-200); ``` `CHAT_MAX` is `200` in ui-core.js:73. The value here would drift if `CHAT_MAX` is changed. **Fix:** Replace with `chatHistory.slice(-CHAT_MAX)`. (`CHAT_MAX` is a global in the same script scope.) **Verdict:** BUILD-NOW · `[CACHE]` **Files:** `js/ui-saves.js` --- ### QA-CONS-7 · [LOW] · `renderCollectibles()` manually builds empty-state HTML instead of `emptyState()` helper ```js container.innerHTML = '<span class="empty-state" style="font-size:11px;">No collectibles registry loaded</span>'; ``` All other render functions call `emptyState(msg)` from ui-core.js. This one duplicates the CSS class and adds an ad-hoc inline style. **Fix:** Replace with `container.innerHTML = emptyState('No collectibles registry loaded');` **Verdict:** BUILD-NOW · `[CACHE]` **Files:** `js/ui-render.js` --- ### QA-CONS-8 · [LOW] · Lincoln memorabilia total hardcoded as `/9` — should be `/defs.length` **ui-render.js:746:** ```js `> LINCOLN MEMORABILIA [${foundCount}/9]` ``` If `FALLOUT_REGISTRY.lincolnMemorabilia` gains an entry, the counter silently shows the wrong total. `renderCollectibles()` correctly uses `total` (= `defs.length`). **Fix:** Replace `9` with `defs.length`. **Verdict:** BUILD-NOW · `[CACHE]` **Files:** `js/ui-render.js` --- ### QA-CONS-9 · [LOW] · `addQuest()` is in ui-saves.js — all other `add*` helpers are in ui-render.js `addItem`, `addAmmo`, `addSquadMember`, `addStatusEffect`, `addPerk`, `addCampaignNote` all live in `ui-render.js`. `addQuest` is the sole outlier in `ui-saves.js`. It follows the same CRUD pattern and belongs with its siblings. **Fix:** Move `addQuest()` to `ui-render.js`. Update any tests that load from a specific file. **Verdict:** BUILD-NOW · `[CACHE][TEST]` **Files:** `js/ui-saves.js`, `js/ui-render.js` --- ### QA-CONS-10 · [LOW] · `alert()` used throughout for user feedback — 30+ call sites, SPEC-FIRST The codebase uses `alert()` extensively for confirmation messages (cloud save success, import success, API key validation). `alert()` is synchronous, blocks the main thread, cannot be styled, breaks the diegetic aesthetic, and on some mobile browsers renders as an OS system dialog with no custom styling. This is a design-level issue. All `alert()` calls should eventually be replaced by `sysModal()` or `appendToChat()` — but this is a significant behavior change requiring UX decisions per call site. **Verdict:** SPEC-FIRST (UX/design decision; do not batch with behavior-neutral refactors) --- ## Section 5 — STRUCTURE --- ### QA-STRUCT-1 · [MEDIUM] · `getSystemDirective()` is a 190-line monolithic template string `api.js:3–193`. The function constructs a single large prompt string with conditional injections for game-specific content, skill systems, faction systems, injection resistance, and schema definitions. There are no named sub-sections or extraction points. The practical problem: adding a new game-specific directive section requires manually placing text in the right position in the string and the effect cannot be unit-tested in isolation. Suites 53/54 test some properties of the output but not the composition logic. **Fix:** Extract named `_directiveSection_*()` helpers that return their sections as strings, then compose in `getSystemDirective()`. This makes each section independently testable. **Verdict:** SPEC-FIRST (the function is behaviorally correct today; refactor would need careful regression testing against the AI contract suite) · **Severity: MEDIUM** **Files:** `js/api.js` --- ### QA-STRUCT-2 · [LOW] · `renderWorldMap()` is 238 lines — the longest function in the codebase Lines 1241–1478. The function handles two zoom levels, zone scoring, visited-zone tracking, badge pip rendering, and HTML generation in one body. It works correctly but is hard to modify in isolation. **Candidate split:** - `_renderWorldGrid(zones, activeZone, opts)` — the 6×6 or 4×4 grid HTML - `_renderZoneDetail(zone, opts)` — the detail view for the active zone **Verdict:** SPEC-FIRST (large function, well-covered by tests; split requires careful boundary definition) · **Severity: LOW** **Files:** `js/ui-render.js` --- ### QA-STRUCT-3 · [LOW] · `autoImportState()` uses `Object.keys(AI_data).forEach()` for lincolnItems **api.js:770:** ```js Object.keys(raw.lincolnItems).forEach(k => { ... }); ``` This is a key-iteration loop over AI-supplied data. Protocol 24 and the Prohibited Patterns table require explicit field mapping — not dynamic key transforms — to prevent AI from injecting unexpected keys into state. The implementation is correctly guarded by a LINCOLN_VOCAB allowlist filter, so no key outside the known set can be accepted. It is a validated loop, not a silent recursive transform. **However:** the pattern is in the same syntactic class as the prohibited recursive transform. A future developer modifying this block might remove the allowlist check without understanding the protocol. The risk is documentation, not current behavior. **Fix:** Add a comment block above the loop: `/* Validated iteration — only keys in LINCOLN_VOCAB are accepted (Protocol 24: explicit mapping required). Do not remove the LINCOLN_VOCAB filter. */` **Verdict:** BUILD-NOW (comment only; no code change) · `[CACHE]` **Files:** `js/api.js` --- ## Section 6 — DOCUMENTATION & STALE REFERENCES --- ### QA-DOC-1 · [HIGH] · `js/registry.js` referenced in 4 core docs — the file does not exist **Impact:** Every Protocol 4, 5, 6, 10, 23 checklist item in CLAUDE.md and RULES.md says "add to `FALLOUT_REGISTRY` in `registry.js`" or "registry.js is read-only." The actual file is `reg_nv.js` (1,942 lines, defines `FALLOUT_REGISTRY` and `registrySearch()`). `registry.js` has never existed as a separate file in the current architecture. **Affected files:** - `CLAUDE.md` — Protocols 4, 5, 6, 23, Architecture Quick Reference (10+ locations) - `RULES.md` — same locations (files are kept identical for protocol sections) - `ARCHITECTURE.md` — load order step 3, ~36KB size claim, entire Section 6 ("Fallout Data Registry"), localStorage table "owned by" column, Mermaid diagram - `README.md` — file structure table, technology stack table, load order - Both test runners — `registrySource` correctly loads `js/reg_nv.js` but 6 test labels say "registry.js" (confusing failure messages) - `state.js:427` — comment says "beforeunload handler in ui.js" (also stale) **Note on test runners:** The runners are functionally correct — they load `reg_nv.js` at line 553 (JS) / line 90 (PS1). The staleness is in comment strings and labels only; no tests fail because of this. **Fix:** Global find-replace `registry.js` → `reg_nv.js` in all four docs; update ARCHITECTURE.md load order step 3 to reflect the split into `reg_nv.js` + `reg_fo3.js`; update test label strings that say "in ui.js" to correctly identify the split file. **Verdict:** BUILD-NOW (docs + test labels only; zero behavior change) · **Severity: HIGH** (every future Protocol 4–6 implementation will follow the wrong checklist step) **Files:** `CLAUDE.md`, `RULES.md`, `ARCHITECTURE.md`, `README.md`, `tests/check-persistence.js`, `tests/check-persistence.ps1` --- ### QA-DOC-2 · [MEDIUM] · ARCHITECTURE.md extensively references `ui.js` — the old monolith ARCHITECTURE.md has 50+ references to `ui.js` (`AudioSettings Cache (ui.js line 6-14)`, `TAB_NAMES declared in ui.js`, `switchTab(tab) // ui.js`, etc.). The actual split is: | Old `ui.js` location | New file | |---|---| | `AudioSettings`, audio functions | `js/ui-audio.js` | | All `render*()` functions, CRUD helpers, faction utilities | `js/ui-render.js` | | Save slots, file import/export, registry autocomplete | `js/ui-saves.js` | | `renderAccount()`, cloud save picker | `js/ui-account.js` | | `loadUI()`, `appendToChat()`, `AudioSettings` init, boot | `js/ui-core.js` | The Mermaid diagram at ARCHITECTURE.md:745 still shows `D[ui.js]` as a single node. **Fix:** Update ARCHITECTURE.md section by section. The load order table, localStorage ownership table, Mermaid diagram, and all function-location annotations. **Verdict:** BUILD-NOW (doc-only update; zero behavior change) · `[TEST]` (both test runners' label strings also say "in ui.js" — update for clarity) **Files:** `ARCHITECTURE.md`, `tests/check-persistence.js`, `tests/check-persistence.ps1` --- ### QA-DOC-3 · [LOW] · `CHAT_MAX` comment stale — says "last 50 written" but code writes 200 **ui-core.js:73:** ```js const CHAT_MAX = 200; // max messages kept in memory; last 50 written to localStorage ``` `appendToChat()` saves the full 200-message `chatHistory` to `localStorage`. The "last 50" figure is from an older implementation. **Fix:** Update comment: `// max messages kept in memory and written to localStorage`. **Verdict:** BUILD-NOW (comment only) · `[CACHE]` **Files:** `js/ui-core.js` --- ### QA-DOC-4 · [LOW] · `gameTimeToTicks()` comment claims "used by time input handler" — wrong The handler calls `calendarToTicks()`, not `gameTimeToTicks()`. **Fix:** This finding is resolved by QA-DEAD-5 (remove the function entirely). If retained, correct the comment. **Verdict:** Resolved by QA-DEAD-5 **Files:** `js/ui-render.js` --- ### QA-DOC-5 · [LOW] · `getRedirectResult` drain path missing Protocol 30 annotation **cloud.js:153:** `getRedirectResult(auth)` is imported and called as a drain-only path for pre-Protocol-30 in-flight redirects. The comment on line 148 partially explains this, but doesn't reference Protocol 30 or state that no new redirect flow should ever be introduced here. **Fix:** Add: `// Drain-only: Protocol 30 bans redirect auth on this project. Do not add new redirect flow here.` **Verdict:** BUILD-NOW (comment only) · `[CACHE]` **Files:** `js/cloud.js` --- ### QA-DOC-6 · [LOW] · ui-audio.js:579 comment says "80ms each" but code stops at 100ms Comment says "80ms each" but `osc.stop(t + 0.1)` is 100ms. **Fix:** Update comment to "100ms each" or change stop time to `t + 0.08`. **Verdict:** BUILD-NOW (comment or constant fix) · `[CACHE]` **Files:** `js/ui-audio.js` --- ## Section 7 — SERVICE WORKER COMPLIANCE (confirmed clean) Both prohibited SW patterns were verified against `sw.js`: | Pattern | Status | |---|---| | `clients.claim()` | **ABSENT** — explicitly omitted with a comment explaining why | | `self.skipWaiting()` inside `install` handler | **ABSENT** — `install` handler comment says "Do NOT call self.skipWaiting() here"; `skipWaiting()` is triggered only by explicit message from main thread (line 77) | Both prohibited patterns are correctly absent. No finding. --- ## Section 8 — LINT / FORMAT **No `eslint-disable` comments anywhere in the codebase.** Zero instances found. The `--max-warnings 0` gate is holding. **`console.log` — zero instances.** Only `console.warn` and `console.error` exist (all in cloud.js, state.js, and ui-core.js for non-fatal error logging with `[RobCo]` prefix). Appropriate usage. **`var` usage — contained and low-risk.** Only `js/ui-saves.js` contains `var` declarations, all within the `initRegistryAutocomplete()` closure (~20 vars). This is legacy code ported as a unit; the vars are function-scoped within the closure. No module-level `var` declarations anywhere. Not a priority. **`alert()` calls — 30+ across api.js, cloud.js, ui-saves.js, state.js, ui-core.js.** Not a lint issue, but noted as QA-CONS-10 (SPEC-FIRST). --- ## Section 9 — PROHIBITED-PATTERN STATUS TABLE Complete cross-check of every entry in the RULES.md Prohibited Patterns table: | Pattern | Status | Finding | |---|---|---| | `clients.claim()` in SW | **ABSENT** ✓ | — | | `self.skipWaiting()` in SW install | **ABSENT** ✓ | — | | `innerHTML +=` inside loops | **VIOLATION** ✗ | QA-PROHIB-1 (api.js:257) | | Untrusted text directly as `innerHTML` | **VIOLATION** ✗ | QA-PROHIB-2 (ui-core.js:158), QA-PROHIB-3 (cloud.js:586) | | `localStorage.getItem()` in audio hot paths | **ABSENT** ✓ | — (ui-audio.js clean) | | Recursive key transform on AI JSON | **PARTIAL** ⚠ | QA-STRUCT-3 (api.js:770 — validated loop, not silent transform; needs comment) | | Silent inventory drops during token triage | Not verifiable in static analysis | — | | Auto-push to cloud on stat changes | **ABSENT** ✓ | — | | Stale test counts in docs | **ABSENT** ✓ | All counts currently in sync | | `linkWithRedirect` / `signInWithRedirect` | **ABSENT** ✓ | — | | Unconditional `signInAnonymously` on boot | **ABSENT** ✓ | Correctly guarded in cloud.js | --- ## Prioritized BUILD-NOW Backlog These are behavior-neutral. Safe to batch and ship in one or two commits. Each can be verified by confirming 1078 tests still pass. ### Batch A — one-liner fixes (bundle together, single commit) | ID | File | Fix | |---|---|---| | QA-DEAD-2 | ui-core.js:575 | Delete `_inputHistory` declaration | | QA-DEAD-3 | api.js:209 | Delete `_cleanKey` declaration | | QA-DEAD-6 | ui-core.js:1099 | Inline `0` in formula, remove `targetDT` | | QA-CONS-4 | cloud.js:289,430 | `|| '2.6.0'` → `|| 'unknown'` | | QA-CONS-5 | api.js:277–279 | Pass `model` directly instead of re-reading localStorage | | QA-CONS-6 | ui-saves.js:103 | `slice(-200)` → `slice(-CHAT_MAX)` | | QA-CONS-7 | ui-render.js:1544 | `±50` → `±5` (user-visible label bug) | | QA-CONS-8 | ui-render.js:746 | `/9` → `/defs.length` | | QA-CONS-9 | ui-render.js:676 | Inline HTML → `emptyState()` helper | | QA-DEAD-8 | api.js:739 | Remove dead gameContext block, add protective comment | | QA-STRUCT-3 | api.js:770 | Add Protocol 24 warning comment on lincolnItems loop | | QA-DOC-3 | ui-core.js:73 | Fix CHAT_MAX comment | | QA-DOC-5 | cloud.js:153 | Add Protocol 30 annotation | | QA-DOC-6 | ui-audio.js:579 | Fix "80ms" → "100ms" comment | | QA-DUP-5 | ui-render.js:275,366 | Hoist `_MONTH_DAYS` to file scope | | QA-DUP-7 | ui-render.js:1658–1928 | Extract `_reg(key)` helper | ### Batch B — small extractions (separate commit, still behavior-neutral) | ID | File | Fix | |---|---|---| | QA-PROHIB-1 | api.js:257 | Replace `innerHTML +=` in loop with single assign | | QA-PROHIB-2 | ui-core.js:158 | `escapeHtml(savedModel)` | | QA-PROHIB-3 | cloud.js:586 | `escapeHtml(data.model)` | | QA-DEAD-1 | ui-audio.js:312 | Remove `showDeltaGhost()` | | QA-DEAD-5 | ui-render.js:357 | Remove `gameTimeToTicks()` | | QA-DEAD-7 | css/terminal.css | Remove `#updateBanner` dead CSS | | QA-DUP-2 | ui-account.js, ui-saves.js | Move `listLocalSaves()` to ui-saves.js | | QA-DUP-3 | ui-core.js:566 | Extract `_startMemCycle()` | | QA-DUP-4 | ui-core.js:554 | Extract `_startUptimeClock()` | | QA-CONS-3 | ui-core.js:1373 | Remove redundant skill-input loop after `renderSkills()` | | QA-CONS-9 | ui-saves.js, ui-render.js | Move `addQuest()` to ui-render.js | | QA-PROHIB-4/5 | api.js:4, 1080–1081 | Cache `playstyle`, `gemini_key`, `gemini_model` | ### Batch C — doc-only updates (single doc commit, not a served file; no cache bump needed) | ID | Files | Fix | |---|---|---| | QA-DOC-1 | CLAUDE.md, RULES.md, ARCHITECTURE.md, README.md, both test runners | Replace `registry.js` references with `reg_nv.js` | | QA-DOC-2 | ARCHITECTURE.md, both test runners | Replace `ui.js` references with split file names | --- ## SPEC-FIRST Backlog These require architectural decisions or non-trivial refactors: | ID | Finding | Why SPEC-FIRST | |---|---|---| | QA-DUP-1 | Extract `_renderReadTracker()` from renderSkillBooks/Magazines | New helper function; must not break sub-panel state persistence | | QA-CONS-2 | Decompose `window.onload` (540 lines) | Touches every boot path; regression risk at every entry point | | QA-CONS-10 | Replace `alert()` with modal/appendToChat | UX behavior change; 30+ call sites, diegetic copy needed for each | | QA-STRUCT-1 | Decompose `getSystemDirective()` into sections | AI contract changes are CRITICAL path (Protocol 14) | | QA-PROHIB-6 | Remove direct `state.*` access from cloud.js | Requires new state accessor API in state.js | | QA-DEAD-4 | `ticksToGameTime()` keep vs. remove decision | Owner call: intentional "public API" keep or dead code removal? | | QA-STRUCT-2 | Split `renderWorldMap()` (238 lines) | Refactor of the most complex render function; map tests are tight | --- ## Constraint Checklist - **[GATE]** 1078 tests must pass · never `--no-verify`. - **[CACHE]** All Batch A and B changes touch served JS/CSS files → `CACHE_NAME` bump required. - **[TEST]** QA-PROHIB-1 and QA-PROHIB-2: add static guards after fixing (the innerHTML+= guard already exists for `uiSource`; extend to cover api.js). - **[DOC-ONLY]** Batch C (registry.js + ui.js doc updates) does not touch served files → no cache bump required. Still requires Protocol 2a test-count sync if any test labels change. - **[BEHAVIOR-NEUTRAL]** None of the BUILD-NOW items changes observable app behavior. The gate's both-runner suite is the safety net — any fix that causes a test failure is not behavior-neutral and must be re-examined before committing. - **[NEVER]** Never bump APP_VERSION for refactoring/cleanup commits. - **[NEVER]** Never use `--no-verify` to bypass the gate. --- *End of CODE_QUALITY_AUDIT.md*