fix(sidebar): hoist _sessionAttentionState to fix ReferenceError crash (#3696) + scope-undef prevention gate (#3698)
Some checks failed
Release & Docker / release (push) Has been cancelled
Some checks failed
Release & Docker / release (push) Has been cancelled
* fix(sidebar): hoist _sessionAttentionState to top-level scope (#3696) _sessionAttentionState was declared inside renderSessionListFromCache() and relied on function hoisting, but the top-level function _sidebarRowHasVisible Messages (reached via renderSessionListFromCache -> _partitionSidebarSessionRows) called it bare. Hoisting is scoped to the enclosing function, so every sidebar cache-render threw 'ReferenceError: _sessionAttentionState is not defined' and the session list went blank. Regressed in #3672 (v0.51.269) when _sidebarRow HasVisibleMessages was extracted to top level. Fix: move _sessionAttentionState to top-level scope (it is pure — only uses its arg plus the i18n global t), so both the visibility predicate and the nested per-row renderer can reach it. Prevention (the durable half): add scripts/scope_undef_gate.py — models the classic-<script> shared global scope (union of all static files' top-level symbols) and runs ESLint no-undef per file, flagging a function defined nested but called from a sibling scope. Wired into CI (.github/workflows/tests.yml lint job) alongside the existing no-const-assign runtime gate, plus an in-suite test (test_static_js_scope_undef.py) and a focused structural regression test (test_issue3696_session_attention_scope.py). RED/GREEN-validated against the broken tree. * fix(streaming): thread source param into stale-stream bailout; tighten scope gate Opus review of #3698 found the new scope_undef_gate's 'source' allowlist entry was masking a real same-class bug: _bailOutOfTerminalEventsFromStaleStream (declared inside attachLiveStream, params activeSid/streamId/uploaded/options) called _closeSource(source) against a 'source' not in its lexical scope. All 5 call sites are inside _wireSSE(source), but JS scope is lexical not dynamic, so the helper would throw ReferenceError: source is not defined on the stale-stream terminal-event path (user back in an active session whose old stream finalizes late). Fix: thread source as an explicit parameter (declaration + all 5 call sites), the same make-the-dependency-explicit fix as #3696 — and REMOVE the 'source' allowlist entry so the gate stays gated against that name (it now passes because the bug is fixed, not because it's allowlisted). Added the documented false-negative classes from Opus's review to the gate docstring (name-collision shadowing, destructuring-regex gap, exposure escape hatches, name-keyed allowlist) and a focused regression test. This is the prevention gate catching a real latent bug on its first outing. --------- Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>
This commit is contained in:
21
.github/workflows/tests.yml
vendored
21
.github/workflows/tests.yml
vendored
@@ -37,6 +37,27 @@ jobs:
|
||||
if: always()
|
||||
run: python3 scripts/ruff_lint.py --all
|
||||
|
||||
# Static-JS runtime-error guards. These catch brick-class bugs that throw only
|
||||
# when the browser executes the code — node --check, source-presence tests, and
|
||||
# the mocked pytest suite all miss them. Two complementary ESLint passes:
|
||||
# * runtime-guard config: no-const-assign / no-import-assign (#3162 class)
|
||||
# * scope_undef_gate.py: no-undef across the shared classic-<script> global
|
||||
# scope, catching a function defined nested but called from a sibling
|
||||
# scope (#3696 — ReferenceError: _sessionAttentionState is not defined).
|
||||
- name: Set up Node for ESLint runtime guards
|
||||
uses: actions/setup-node@v4
|
||||
with:
|
||||
node-version: '20'
|
||||
|
||||
- name: Install ESLint
|
||||
run: npm install --no-save eslint@^10
|
||||
|
||||
- name: ESLint runtime-error gate (no-const-assign / no-import-assign, #3162)
|
||||
run: npx eslint --no-config-lookup -c eslint.runtime-guard.config.mjs "static/**/*.js"
|
||||
|
||||
- name: Scope / undefined-reference gate (#3696)
|
||||
run: python3 scripts/scope_undef_gate.py
|
||||
|
||||
test:
|
||||
runs-on: ubuntu-latest
|
||||
strategy:
|
||||
|
||||
@@ -3,6 +3,13 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
- **Sidebar no longer crashes with `ReferenceError: _sessionAttentionState is not defined`.** The session-attention helper was declared *inside* `renderSessionListFromCache()` and relied on function hoisting, but the top-level `_sidebarRowHasVisibleMessages` (reached via `renderSessionListFromCache` → `_partitionSidebarSessionRows`) called it bare — and hoisting is scoped to the enclosing function, so every sidebar cache-render threw and the session list went blank. `_sessionAttentionState` is now a top-level function reachable by both call sites. Regressed in #3672 (v0.51.269). (#3696)
|
||||
- **Stale-stream terminal events no longer risk a `ReferenceError: source is not defined`.** `_bailOutOfTerminalEventsFromStaleStream` (declared inside `attachLiveStream`) called `_closeSource(source)` against a `source` that was not in its lexical scope — it would have thrown on the late-finalizing-stream path when the user is back in an active session. `source` is now threaded as an explicit parameter. Found by the new scope gate below during review. (#3696)
|
||||
|
||||
### Internal
|
||||
- **New static-JS scope/undefined-reference gate (`scripts/scope_undef_gate.py`).** Models the WebUI's classic-`<script>` shared global scope and runs ESLint `no-undef` per file, flagging a function that is defined only *nested* but called from a sibling/top-level scope — the brick class behind #3696 that `node --check`, source-presence tests, and the existing `no-const-assign` runtime gate all miss. Wired into the CI `lint` job alongside the `no-const-assign`/`no-import-assign` runtime gate, with an in-suite test (`tests/test_static_js_scope_undef.py`) and a focused structural regression test (`tests/test_issue3696_session_attention_scope.py`). (#3696)
|
||||
|
||||
## [v0.51.288] — 2026-06-06 — Release JD (stage-r24 — collapsible approval card)
|
||||
|
||||
### Added
|
||||
|
||||
189
scripts/scope_undef_gate.py
Normal file
189
scripts/scope_undef_gate.py
Normal file
@@ -0,0 +1,189 @@
|
||||
#!/usr/bin/env python3
|
||||
"""scope_undef_gate.py — catch the "ReferenceError: X is not defined" brick class.
|
||||
|
||||
The WebUI front-end ships as classic (non-module) ``<script>`` tags that all share
|
||||
ONE implicit global scope. A function declared *inside* another function is NOT a
|
||||
global, so calling it (un-guarded) from a different top-level function throws
|
||||
``ReferenceError`` at runtime — but only when that code path actually executes in
|
||||
the browser. ``node --check`` (syntax only), source-presence tests, and the
|
||||
existing ``no-const-assign`` ESLint runtime gate all MISS it.
|
||||
|
||||
Canonical bug — #3696 (v0.51.269 regression): ``_sessionAttentionState`` was
|
||||
declared *inside* ``renderSessionListFromCache()`` and relied on "function
|
||||
hoisting", but a *separate* top-level function ``_sidebarRowHasVisibleMessages``
|
||||
called it bare. Hoisting is scoped to the enclosing function, so every sidebar
|
||||
cache-render crashed with ``_sessionAttentionState is not defined`` and the
|
||||
session list went blank.
|
||||
|
||||
How the gate works (and why it has no cross-file false positives):
|
||||
1. It scans every static ``*.js`` file for the union of all TOP-LEVEL symbols
|
||||
(``function NAME``, top-level ``const/let/var``, and ``window.NAME = ...``).
|
||||
That union IS the real shared global namespace at runtime.
|
||||
2. It lints each file individually with ESLint ``no-undef``, supplying that
|
||||
union (plus browser/library builtins) as ``globals``. Cross-file references
|
||||
(``api``, ``loadSession``, ``renderSessionList`` …) resolve cleanly because
|
||||
they ARE top-level somewhere; meanwhile ESLint's per-file scope analysis
|
||||
still flags a name that is *defined only nested* and called from a sibling
|
||||
scope in the same file — the #3696 shape.
|
||||
3. A short, documented allowlist covers names that are legitimately dynamic and
|
||||
ESLint can't see are safe: helpers exposed via ``window.NAME = ...`` and
|
||||
called bare elsewhere, and OPTIONAL helpers always called behind a
|
||||
``typeof NAME === 'function'`` guard (those can't throw — the guard is the
|
||||
contract). The un-guarded bare call is exactly what makes #3696 a bug.
|
||||
|
||||
Usage:
|
||||
python3 scripts/scope_undef_gate.py [/path/to/webui/checkout]
|
||||
(defaults to the repo this script lives in)
|
||||
|
||||
Exit 0 = clean (or eslint unavailable → skip). Non-zero = a new undefined /
|
||||
scope-misplaced reference that throws at runtime. DO NOT TAG/RELEASE on non-zero.
|
||||
|
||||
Known false-negative classes (the gate is a strong net, not a proof):
|
||||
* Name-collision shadowing — if two files both declare a top-level symbol of the
|
||||
same name, the union includes it, so a bare cross-scope call to that name goes
|
||||
unflagged (runtime binds to the other file's symbol = a wrong-function bug, not
|
||||
a ReferenceError). A duplicate top-level symbol across files is itself a smell.
|
||||
* Non-`,;`-terminated top-level destructuring (`const {a, b} = x` where `b` ends
|
||||
the line) is not captured by the symbol regex below. None exist in the bundle
|
||||
today; revisit the regex if that pattern is introduced.
|
||||
* Exposure escape hatches not scanned: `globalThis.X = X`, `Object.assign(window,
|
||||
{X})`, `(0,eval)(...)`. None used today.
|
||||
* The allowlist is keyed by NAME, not call-site — an entry green-lights every bare
|
||||
reference to that name everywhere. Only add an entry when EVERY call site is a
|
||||
`window.X =` exposure or a `typeof X === 'function'` guard; otherwise fix the
|
||||
bug (hoist / pass the value as a param), don't allowlist it. (#3696 review:
|
||||
a `source` allowlist entry was masking a real same-class bug at messages.js:876,
|
||||
fixed by threading `source` as a parameter instead of allowlisting.)
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import re
|
||||
import subprocess
|
||||
import sys
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
from shutil import which
|
||||
|
||||
# Browser + ECMAScript builtins + CDN libraries loaded before our bundle. Explicit
|
||||
# (not an eslint "env") so the gate is self-contained and reviewable.
|
||||
BROWSER_GLOBALS = [
|
||||
"window", "document", "console", "localStorage", "sessionStorage", "setTimeout",
|
||||
"clearTimeout", "setInterval", "clearInterval", "requestAnimationFrame",
|
||||
"cancelAnimationFrame", "queueMicrotask", "reportError", "fetch", "URL",
|
||||
"URLSearchParams", "Blob", "File", "FileList", "FileReader", "FormData", "navigator",
|
||||
"location", "history", "alert", "prompt", "confirm", "EventSource", "WebSocket",
|
||||
"BroadcastChannel", "Image", "Audio", "MediaRecorder", "speechSynthesis",
|
||||
"SpeechSynthesisUtterance", "AudioContext", "webkitAudioContext", "MutationObserver",
|
||||
"IntersectionObserver", "ResizeObserver", "DataTransfer", "DragEvent", "Event",
|
||||
"CustomEvent", "KeyboardEvent", "MouseEvent", "PointerEvent", "TouchEvent",
|
||||
"WheelEvent", "ClipboardEvent", "getComputedStyle", "matchMedia", "atob", "btoa",
|
||||
"structuredClone", "crypto", "performance", "screen", "DOMParser", "Node", "NodeList",
|
||||
"HTMLElement", "Element", "Text", "AbortController", "AbortSignal", "TextDecoder",
|
||||
"TextEncoder", "caches", "self", "CSS", "Notification", "Response", "Request",
|
||||
"Headers", "getSelection", "scrollTo", "scrollBy", "postMessage", "indexedDB",
|
||||
# ECMAScript builtins
|
||||
"Promise", "Map", "Set", "WeakMap", "WeakSet", "Symbol", "Proxy", "Reflect", "JSON",
|
||||
"Math", "Date", "RegExp", "Array", "Object", "String", "Number", "Boolean", "Error",
|
||||
"TypeError", "RangeError", "Intl", "BigInt", "Uint8Array", "Int32Array",
|
||||
"Float64Array", "ArrayBuffer", "DataView", "Function", "parseInt", "parseFloat",
|
||||
"isNaN", "isFinite", "encodeURIComponent", "decodeURIComponent", "encodeURI",
|
||||
"decodeURI", "globalThis",
|
||||
# CDN libraries loaded via <script> before our bundle
|
||||
"Prism", "mermaid", "katex", "hljs", "jsyaml", "Terminal", "FitAddon", "WebLinksAddon",
|
||||
]
|
||||
|
||||
# Names ESLint can't statically prove are safe, verified NON-bugs. Each MUST be one of:
|
||||
# (a) exposed via `window.NAME = ...` (often inside an IIFE) and called bare, or
|
||||
# (b) an OPTIONAL helper ALWAYS called behind `typeof NAME === 'function'` (the
|
||||
# guard means a missing binding is a no-op, never a throw), or
|
||||
# (c) a closure variable from an enclosing function scope ESLint can't bind per-file.
|
||||
# Keep this SHORT and justified. A bare (un-guarded) call to a nested-only function is
|
||||
# a real bug and must NOT be added here — hoist the function to top level instead.
|
||||
PROJECT_DYNAMIC_GLOBALS = {
|
||||
"placeLiveToolCardsHost": "typeof-guarded optional (ui/sessions/messages call sites)",
|
||||
"watchInflightSession": "typeof-guarded optional fallback (sessions.js)",
|
||||
"_applyMediaPlaybackPreferences": "typeof-guarded optional (ui.js / workspace.js)",
|
||||
}
|
||||
|
||||
|
||||
def _toplevel_symbols(src: str) -> set[str]:
|
||||
syms: set[str] = set()
|
||||
syms |= set(re.findall(r"^function\s+([A-Za-z_$][\w$]*)", src, re.M))
|
||||
syms |= set(re.findall(r"^async\s+function\s+([A-Za-z_$][\w$]*)", src, re.M))
|
||||
for m in re.finditer(r"^(?:const|let|var)\s+(.+)", src, re.M):
|
||||
decl = m.group(1)
|
||||
for name in re.findall(r"([A-Za-z_$][\w$]*)\s*[=;,]", decl):
|
||||
syms.add(name)
|
||||
lead = re.match(r"([A-Za-z_$][\w$]*)", decl)
|
||||
if lead:
|
||||
syms.add(lead.group(1))
|
||||
syms |= set(re.findall(r"window\.([A-Za-z_$][\w$]*)\s*=", src))
|
||||
return syms
|
||||
|
||||
|
||||
def main() -> int:
|
||||
root = Path(sys.argv[1]).resolve() if len(sys.argv) > 1 else Path(__file__).resolve().parent.parent
|
||||
static_dir = root / "static"
|
||||
eslint = which("eslint")
|
||||
if not eslint:
|
||||
print("⚠ eslint not found on PATH — scope_undef_gate SKIPPED (install eslint to enable).")
|
||||
return 0
|
||||
if not static_dir.is_dir():
|
||||
print(f"❌ static dir not found at {static_dir}")
|
||||
return 2
|
||||
|
||||
files = [f for f in sorted(static_dir.glob("*.js")) if not f.name.endswith(".min.js")]
|
||||
project_syms: set[str] = set()
|
||||
for f in files:
|
||||
project_syms |= _toplevel_symbols(f.read_text(encoding="utf-8"))
|
||||
|
||||
allow = project_syms | set(BROWSER_GLOBALS) | set(PROJECT_DYNAMIC_GLOBALS)
|
||||
globals_obj = "{" + ",".join(f'"{n}":"readonly"' for n in sorted(allow)) + "}"
|
||||
|
||||
findings: list[tuple[str, int, str]] = []
|
||||
with tempfile.TemporaryDirectory() as td:
|
||||
config_path = Path(td) / "scope.config.mjs"
|
||||
config_path.write_text(
|
||||
"export default [{files:[\"**/*.js\"],"
|
||||
"languageOptions:{ecmaVersion:\"latest\",sourceType:\"script\","
|
||||
f"globals:{globals_obj}}},"
|
||||
"rules:{\"no-undef\":\"error\"}}];",
|
||||
encoding="utf-8",
|
||||
)
|
||||
for f in files:
|
||||
proc = subprocess.run(
|
||||
[eslint, "--no-config-lookup", "-c", str(config_path), "-f", "json", str(f)],
|
||||
capture_output=True, text=True,
|
||||
)
|
||||
try:
|
||||
report = json.loads(proc.stdout)
|
||||
except json.JSONDecodeError:
|
||||
print(f"❌ eslint failed on {f.name}:\n" + (proc.stderr or proc.stdout)[:1500])
|
||||
return 2
|
||||
for file_report in report:
|
||||
for msg in file_report.get("messages", []):
|
||||
if msg.get("ruleId") == "no-undef":
|
||||
findings.append((f.name, msg.get("line", 0), msg.get("message", "")))
|
||||
|
||||
if not findings:
|
||||
print(f"✅ scope_undef_gate: CLEAN ({len(files)} static files, "
|
||||
f"{len(project_syms)} project globals, no undefined references).")
|
||||
return 0
|
||||
|
||||
print("❌ scope_undef_gate FAILED — undefined reference(s) that throw at runtime in the "
|
||||
"browser (brick class, see #3696):\n")
|
||||
for src_file, line, message in findings:
|
||||
print(f" {src_file}:{line}: {message}")
|
||||
print(
|
||||
"\nA flagged name is a REAL bug when it is a function defined inside another\n"
|
||||
"function and called BARE from a sibling/top-level scope — hoist it to top\n"
|
||||
"level (the #3696 fix). It is only safe to add to PROJECT_DYNAMIC_GLOBALS in\n"
|
||||
"this script (with a one-line justification) if every call site is either a\n"
|
||||
"`window.NAME = ...` exposure or a `typeof NAME === 'function'` guard."
|
||||
)
|
||||
return 1
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
raise SystemExit(main())
|
||||
@@ -871,7 +871,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){
|
||||
function _ownsActiveStreamOrBackground(){
|
||||
return !_isActiveSession() || S.activeStreamId===streamId;
|
||||
}
|
||||
function _bailOutOfTerminalEventsFromStaleStream(){
|
||||
function _bailOutOfTerminalEventsFromStaleStream(source){
|
||||
if(_ownsActiveStreamOrBackground()) return false;
|
||||
_closeSource(source);
|
||||
return true;
|
||||
@@ -2094,7 +2094,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){
|
||||
|
||||
source.addEventListener('done',e=>{
|
||||
if(_streamFinalized) return;
|
||||
if(_bailOutOfTerminalEventsFromStaleStream()) return;
|
||||
if(_bailOutOfTerminalEventsFromStaleStream(source)) return;
|
||||
// Set _streamFinalized IMMEDIATELY — before any fade delay. Without this,
|
||||
// a stream_end event arriving during the fade window sees
|
||||
// _streamFinalized=false, calls _restoreSettledSession(), and overwrites
|
||||
@@ -2319,7 +2319,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){
|
||||
_closeSource(source);
|
||||
return;
|
||||
}
|
||||
if(_bailOutOfTerminalEventsFromStaleStream()) return;
|
||||
if(_bailOutOfTerminalEventsFromStaleStream(source)) return;
|
||||
_terminalStateReached=true;
|
||||
try{
|
||||
const d=JSON.parse(e.data||'{}');
|
||||
@@ -2466,7 +2466,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){
|
||||
});
|
||||
|
||||
source.addEventListener('apperror',e=>{
|
||||
if(_bailOutOfTerminalEventsFromStaleStream()) return;
|
||||
if(_bailOutOfTerminalEventsFromStaleStream(source)) return;
|
||||
_terminalStateReached=true;
|
||||
if(_persistTimer){clearTimeout(_persistTimer);_persistTimer=null;}
|
||||
_streamFinalized=true;
|
||||
@@ -2542,7 +2542,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){
|
||||
});
|
||||
|
||||
source.addEventListener('error',async e=>{
|
||||
if(_bailOutOfTerminalEventsFromStaleStream() && !_streamFinalized){
|
||||
if(_bailOutOfTerminalEventsFromStaleStream(source) && !_streamFinalized){
|
||||
return;
|
||||
}
|
||||
if(_terminalStateReached || _streamFinalized){
|
||||
@@ -2595,7 +2595,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){
|
||||
});
|
||||
|
||||
source.addEventListener('cancel',e=>{
|
||||
if(_bailOutOfTerminalEventsFromStaleStream()) return;
|
||||
if(_bailOutOfTerminalEventsFromStaleStream(source)) return;
|
||||
_terminalStateReached=true;
|
||||
if(_persistTimer){clearTimeout(_persistTimer);_persistTimer=null;}
|
||||
_streamFinalized=true;
|
||||
|
||||
@@ -4170,6 +4170,29 @@ function _resyncSessionVirtualWindowAfterRender(list, expectedScrollTop, virtual
|
||||
});
|
||||
}
|
||||
|
||||
// Top-level so BOTH the sidebar visibility predicate (_sidebarRowHasVisibleMessages,
|
||||
// reached via renderSessionListFromCache -> _partitionSidebarSessionRows) and the
|
||||
// per-row renderer (_renderOneSession, nested in renderSessionListFromCache) can call
|
||||
// it. It was previously declared INSIDE renderSessionListFromCache and relied on
|
||||
// function hoisting — but hoisting is scoped to the enclosing function, so the
|
||||
// top-level _sidebarRowHasVisibleMessages threw "ReferenceError: _sessionAttentionState
|
||||
// is not defined" on every cache render, crashing the sidebar (#3696, regressed in
|
||||
// #3672 when _sidebarRowHasVisibleMessages was extracted to top level). Pure function
|
||||
// (only its arg `s` plus the i18n global `t`), so hoisting it is safe.
|
||||
function _sessionAttentionState(s){
|
||||
const attention=s&&s.attention&&typeof s.attention==='object'?s.attention:null;
|
||||
if(!attention||!attention.kind||!Number.isFinite(Number(attention.count))||Number(attention.count)<=0)return null;
|
||||
const kind=String(attention.kind)==='approval'?'approval':(String(attention.kind)==='clarify'?'clarify':'attention');
|
||||
const count=Math.max(1,Number(attention.count)||1);
|
||||
const labelKey=kind==='approval'?'session_attention_approval':(kind==='clarify'?'session_attention_clarify':'session_attention_generic');
|
||||
const titleKey=kind==='approval'?'session_attention_approval_title':(kind==='clarify'?'session_attention_clarify_title':'session_attention_generic_title');
|
||||
const fallback=kind==='approval'?(count===1?'Approval':`${count} approvals`):(kind==='clarify'?(count===1?'Question':`${count} questions`):(count===1?'Attention':`${count} items`));
|
||||
const titleFallback=kind==='approval'?'Waiting for permission decision':(kind==='clarify'?'Waiting for your answer':'Waiting for user action');
|
||||
const label=(typeof t==='function')?t(labelKey,count):fallback;
|
||||
const title=(typeof t==='function')?t(titleKey,count):titleFallback;
|
||||
return {kind,count,severity:String(attention.severity||''),label,title};
|
||||
}
|
||||
|
||||
function _sidebarRowHasVisibleMessages(s, activeSidForSidebar){
|
||||
return (s.message_count||0)>0 ||
|
||||
_sessionAttentionState(s) ||
|
||||
@@ -4523,20 +4546,6 @@ function renderSessionListFromCache(){
|
||||
const reflowTimeout=animateRefresh?SESSION_LIST_FLIP_TIMEOUT_MS:SESSION_REFLOW_TIMEOUT_MS;
|
||||
_pendingSessionReflowPositions=null;
|
||||
_playSessionRowsReflowFromPositions(reflowBefore,reflowTimeout,_sessionPrefersReducedMotion);
|
||||
// Note: declared after the groups loop but available via function hoisting.
|
||||
function _sessionAttentionState(s){
|
||||
const attention=s&&s.attention&&typeof s.attention==='object'?s.attention:null;
|
||||
if(!attention||!attention.kind||!Number.isFinite(Number(attention.count))||Number(attention.count)<=0)return null;
|
||||
const kind=String(attention.kind)==='approval'?'approval':(String(attention.kind)==='clarify'?'clarify':'attention');
|
||||
const count=Math.max(1,Number(attention.count)||1);
|
||||
const labelKey=kind==='approval'?'session_attention_approval':(kind==='clarify'?'session_attention_clarify':'session_attention_generic');
|
||||
const titleKey=kind==='approval'?'session_attention_approval_title':(kind==='clarify'?'session_attention_clarify_title':'session_attention_generic_title');
|
||||
const fallback=kind==='approval'?(count===1?'Approval':`${count} approvals`):(kind==='clarify'?(count===1?'Question':`${count} questions`):(count===1?'Attention':`${count} items`));
|
||||
const titleFallback=kind==='approval'?'Waiting for permission decision':(kind==='clarify'?'Waiting for your answer':'Waiting for user action');
|
||||
const label=(typeof t==='function')?t(labelKey,count):fallback;
|
||||
const title=(typeof t==='function')?t(titleKey,count):titleFallback;
|
||||
return {kind,count,severity:String(attention.severity||''),label,title};
|
||||
}
|
||||
|
||||
function _renderOneSession(s, isPinnedGroup=false){
|
||||
const el=document.createElement('div');
|
||||
|
||||
83
tests/test_issue3696_session_attention_scope.py
Normal file
83
tests/test_issue3696_session_attention_scope.py
Normal file
@@ -0,0 +1,83 @@
|
||||
"""Regression test for #3696 — `_sessionAttentionState is not defined`.
|
||||
|
||||
Bug: `_sessionAttentionState` was declared INSIDE `renderSessionListFromCache()`
|
||||
and relied on "function hoisting", but the separate top-level function
|
||||
`_sidebarRowHasVisibleMessages` (reached via renderSessionListFromCache ->
|
||||
_partitionSidebarSessionRows) called it BARE. Function hoisting is scoped to the
|
||||
enclosing function, so the call threw `ReferenceError: _sessionAttentionState is
|
||||
not defined` on every sidebar cache-render — the session list went blank
|
||||
(v0.51.269, regressed by #3672 when _sidebarRowHasVisibleMessages was extracted
|
||||
to top level).
|
||||
|
||||
Fix: hoist `_sessionAttentionState` to top-level (module/global) scope so both the
|
||||
top-level visibility predicate and the nested per-row renderer can reach it.
|
||||
|
||||
This is a structural test (no node/eslint needed, runs in every shard). The
|
||||
behavioral scope-analysis guard lives in tests/test_static_js_scope_undef.py +
|
||||
scripts/scope_undef_gate.py, which catch the whole class. This test pins the
|
||||
specific #3696 invariant cheaply.
|
||||
"""
|
||||
import re
|
||||
from pathlib import Path
|
||||
|
||||
SESSIONS_JS = (Path(__file__).resolve().parents[1] / "static" / "sessions.js").read_text(encoding="utf-8")
|
||||
|
||||
|
||||
def _brace_body(src: str, open_brace_idx: int) -> tuple[int, int]:
|
||||
"""Return (start, end) char offsets of the body delimited by the brace at
|
||||
open_brace_idx (exclusive of the braces)."""
|
||||
depth = 1
|
||||
i = open_brace_idx + 1
|
||||
while i < len(src) and depth:
|
||||
if src[i] == "{":
|
||||
depth += 1
|
||||
elif src[i] == "}":
|
||||
depth -= 1
|
||||
i += 1
|
||||
return open_brace_idx + 1, i - 1
|
||||
|
||||
|
||||
def _function_span(src: str, name: str) -> tuple[int, int]:
|
||||
m = re.search(r"function\s+" + re.escape(name) + r"\s*\(", src)
|
||||
assert m, f"function {name} not found"
|
||||
brace = src.find("{", m.end())
|
||||
return _brace_body(src, brace)
|
||||
|
||||
|
||||
def test_session_attention_state_is_top_level():
|
||||
"""`_sessionAttentionState` must be declared at top-level scope (column 0),
|
||||
not nested inside another function — otherwise the top-level callers throw
|
||||
ReferenceError (#3696)."""
|
||||
decls = re.findall(r"^(\s*)function\s+_sessionAttentionState\s*\(", SESSIONS_JS, re.M)
|
||||
assert decls, "_sessionAttentionState declaration not found"
|
||||
assert len(decls) == 1, f"expected exactly one declaration, found {len(decls)}"
|
||||
assert decls[0] == "", (
|
||||
"#3696: _sessionAttentionState must be a TOP-LEVEL function (no leading "
|
||||
f"indentation), but it is indented {len(decls[0])} spaces (nested). A nested "
|
||||
"declaration only hoists within its enclosing function, so top-level callers "
|
||||
"like _sidebarRowHasVisibleMessages throw 'ReferenceError: _sessionAttentionState "
|
||||
"is not defined'."
|
||||
)
|
||||
|
||||
|
||||
def test_session_attention_state_not_nested_in_render_from_cache():
|
||||
"""Belt-and-suspenders: the definition must NOT live inside the body of
|
||||
`renderSessionListFromCache` (where it was when #3696 shipped)."""
|
||||
start, end = _function_span(SESSIONS_JS, "renderSessionListFromCache")
|
||||
body = SESSIONS_JS[start:end]
|
||||
assert "function _sessionAttentionState(" not in body, (
|
||||
"#3696: _sessionAttentionState is defined inside renderSessionListFromCache() — "
|
||||
"it must be hoisted to top-level so _sidebarRowHasVisibleMessages can call it."
|
||||
)
|
||||
|
||||
|
||||
def test_sidebar_visibility_predicate_calls_attention_state():
|
||||
"""Guard the regression's trigger: _sidebarRowHasVisibleMessages (top-level)
|
||||
references _sessionAttentionState. If this call is ever removed the bug can't
|
||||
recur, but while it exists the function MUST be top-level (asserted above)."""
|
||||
start, end = _function_span(SESSIONS_JS, "_sidebarRowHasVisibleMessages")
|
||||
body = SESSIONS_JS[start:end]
|
||||
assert "_sessionAttentionState(" in body, (
|
||||
"_sidebarRowHasVisibleMessages no longer calls _sessionAttentionState — if this "
|
||||
"is intentional, update this test; the #3696 invariant assumes this call exists."
|
||||
)
|
||||
56
tests/test_messages_stale_stream_source_scope.py
Normal file
56
tests/test_messages_stale_stream_source_scope.py
Normal file
@@ -0,0 +1,56 @@
|
||||
"""Regression test for the messages.js stale-stream `source` scope bug.
|
||||
|
||||
Surfaced by the scope_undef_gate (scripts/scope_undef_gate.py) during the #3696
|
||||
review: `_bailOutOfTerminalEventsFromStaleStream` is declared at brace depth 2
|
||||
inside `attachLiveStream` (whose params are activeSid/streamId/uploaded/options —
|
||||
no `source`), yet its body called `_closeSource(source)` referencing a `source`
|
||||
that is NOT in its lexical scope. All call sites live inside `_wireSSE(source)`,
|
||||
but JS scoping is lexical not dynamic, so when the helper runs it would throw
|
||||
`ReferenceError: source is not defined` on the stale-stream terminal-event path
|
||||
(`_ownsActiveStreamOrBackground()` false → user back in an active session whose
|
||||
old stream finalizes late). Same class as #3696.
|
||||
|
||||
Fix: thread `source` as an explicit parameter
|
||||
(`_bailOutOfTerminalEventsFromStaleStream(source)`) and pass it at every call
|
||||
site, instead of relying on (broken) scope resolution. This test locks that the
|
||||
declaration takes the param and no bare-call site remains.
|
||||
"""
|
||||
import re
|
||||
from pathlib import Path
|
||||
|
||||
MESSAGES_JS = (Path(__file__).resolve().parents[1] / "static" / "messages.js").read_text(encoding="utf-8")
|
||||
|
||||
|
||||
def test_bailout_helper_takes_source_param():
|
||||
"""The helper must declare a `source` parameter (not rely on an out-of-scope
|
||||
closure variable)."""
|
||||
m = re.search(r"function\s+_bailOutOfTerminalEventsFromStaleStream\s*\(([^)]*)\)", MESSAGES_JS)
|
||||
assert m, "_bailOutOfTerminalEventsFromStaleStream declaration not found"
|
||||
params = [p.strip() for p in m.group(1).split(",") if p.strip()]
|
||||
assert "source" in params, (
|
||||
"_bailOutOfTerminalEventsFromStaleStream must take `source` as a parameter — "
|
||||
"it calls _closeSource(source) but is declared inside attachLiveStream (no "
|
||||
"`source` in scope), so a bare reference throws ReferenceError on the "
|
||||
f"stale-stream path. Current params: {params}"
|
||||
)
|
||||
|
||||
|
||||
def test_no_bare_bailout_call_sites_remain():
|
||||
"""Every call site must pass `source` — a bare `()` call would leave the helper's
|
||||
`source` undefined again."""
|
||||
bare = re.findall(r"_bailOutOfTerminalEventsFromStaleStream\(\s*\)", MESSAGES_JS)
|
||||
assert not bare, (
|
||||
f"Found {len(bare)} bare _bailOutOfTerminalEventsFromStaleStream() call site(s) "
|
||||
"with no argument — each must pass `source` so the helper can close the right "
|
||||
"stream. A bare call reintroduces the ReferenceError."
|
||||
)
|
||||
|
||||
|
||||
def test_bailout_call_sites_pass_source():
|
||||
"""Positive check: the call sites pass `source`."""
|
||||
calls = re.findall(r"_bailOutOfTerminalEventsFromStaleStream\(\s*source\s*\)", MESSAGES_JS)
|
||||
assert len(calls) >= 5, (
|
||||
f"expected >=5 call sites passing `source`, found {len(calls)} — if the SSE "
|
||||
"terminal-event wiring changed, update this count, but every call must still "
|
||||
"pass source."
|
||||
)
|
||||
@@ -178,7 +178,11 @@ class TestProfileSessionListFlip:
|
||||
|
||||
def test_profile_refresh_drops_queued_reflow_before_playing_flip(self):
|
||||
start = self.JS.index("// Refresh FLIP and queued archive/delete reflow both drive")
|
||||
end = self.JS.index("// Note: declared after the groups loop", start)
|
||||
# End anchor: the next function declaration after the reflow block. (Was the
|
||||
# "// Note: declared after the groups loop" comment on the nested
|
||||
# _sessionAttentionState, which #3696 removed when that helper was hoisted to
|
||||
# top-level scope — so anchor on the stable _renderOneSession decl instead.)
|
||||
end = self.JS.index("function _renderOneSession(", start)
|
||||
block = self.JS[start:end]
|
||||
|
||||
assert "const reflowBefore=animateRefresh?flipBefore:_pendingSessionReflowPositions;" in block
|
||||
|
||||
58
tests/test_static_js_scope_undef.py
Normal file
58
tests/test_static_js_scope_undef.py
Normal file
@@ -0,0 +1,58 @@
|
||||
"""Scope / undefined-reference guard for the static JS bundle (issue #3696).
|
||||
|
||||
Why this exists: #3696 was a brick-class regression — `_sessionAttentionState`
|
||||
was declared *inside* `renderSessionListFromCache()` but called (un-guarded) from
|
||||
a separate top-level function `_sidebarRowHasVisibleMessages`. Function hoisting is
|
||||
scoped to the enclosing function, so the call threw
|
||||
`ReferenceError: _sessionAttentionState is not defined` on every sidebar
|
||||
cache-render and the session list went blank (v0.51.269, regressed by #3672).
|
||||
|
||||
Nothing caught it: `node --check` is a syntax check (a nested function IS valid
|
||||
syntax), source-presence tests asserted the strings existed (they did — in the
|
||||
wrong scope), and the existing `no-const-assign` runtime gate only covers
|
||||
const-reassign / import-assign. This is a DIFFERENT runtime-error class:
|
||||
referencing a name that isn't in scope.
|
||||
|
||||
`scripts/scope_undef_gate.py` models the WebUI's classic-`<script>` shared global
|
||||
scope (all top-level symbols across every static file become one namespace), then
|
||||
runs ESLint `no-undef` per file. Cross-file globals resolve; a function defined
|
||||
only nested and called from a sibling scope is flagged. See that script's header
|
||||
for the full design + the verified dynamic-global allowlist.
|
||||
|
||||
Graceful skip: if node or eslint isn't available the test SKIPS (the release gate
|
||||
runs where eslint IS installed — see TESTING.md), so toolchain-free envs aren't
|
||||
blocked.
|
||||
"""
|
||||
import shutil
|
||||
import subprocess
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
REPO = Path(__file__).resolve().parents[1]
|
||||
GATE = REPO / "scripts" / "scope_undef_gate.py"
|
||||
|
||||
|
||||
@pytest.mark.skipif(not GATE.exists(), reason="scope_undef_gate.py missing")
|
||||
def test_static_js_has_no_undefined_references():
|
||||
if shutil.which("eslint") is None:
|
||||
pytest.skip(
|
||||
"eslint not installed — install with "
|
||||
"`npm install --no-save --before=<48h-ago> eslint` to enforce the "
|
||||
"scope/undef guard locally (CI/release env has it). See TESTING.md."
|
||||
)
|
||||
if shutil.which("node") is None:
|
||||
pytest.skip("node not available")
|
||||
|
||||
proc = subprocess.run(
|
||||
[sys.executable, str(GATE), str(REPO)],
|
||||
capture_output=True, text=True, timeout=180,
|
||||
)
|
||||
# Exit 0 = clean or eslint-skip; 1 = real finding; 2 = setup error.
|
||||
assert proc.returncode != 1, (
|
||||
"scope_undef_gate found undefined reference(s) that throw at runtime in the "
|
||||
"browser (brick-class, see #3696):\n" + proc.stdout + proc.stderr
|
||||
)
|
||||
if proc.returncode == 2:
|
||||
pytest.skip(f"scope_undef_gate setup issue (not a code failure): {proc.stdout[:300]}")
|
||||
Reference in New Issue
Block a user