path: root/review-glm5.2.html

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>newOS4 Code Review — GLM 5.2</title>
<style>
:root {
  --bg: #0a0e17;
  --bg2: #111827;
  --border: #1e293b;
  --text: #e2e8f0;
  --text-muted: #94a3b8;
  --critical: #ef4444;
  --high: #f97316;
  --medium: #eab308;
  --low: #22c55e;
  --accent: #6366f1;
  --accent2: #8b5cf6;
}
* { margin: 0; padding: 0; box-sizing: border-box; }
body {
  background: var(--bg);
  color: var(--text);
  font-family: 'Inter', -apple-system, BlinkMacSystemFont, sans-serif;
  line-height: 1.6;
  padding: 2rem;
  max-width: 960px;
  margin: 0 auto;
}
h1 { font-size: 2rem; margin-bottom: 0.5rem; background: linear-gradient(135deg, var(--accent), var(--accent2)); -webkit-background-clip: text; -webkit-text-fill-color: transparent; }
.subtitle { color: var(--text-muted); margin-bottom: 2rem; font-size: 0.9rem; }
h2 { font-size: 1.3rem; margin: 2rem 0 1rem; padding-bottom: 0.5rem; border-bottom: 1px solid var(--border); color: var(--accent); }
h3 { font-size: 1.1rem; margin: 1.5rem 0 0.5rem; color: var(--text); }
.severity { display: inline-block; padding: 2px 8px; border-radius: 4px; font-size: 0.75rem; font-weight: 600; text-transform: uppercase; letter-spacing: 0.5px; }
.critical { background: rgba(239,68,68,0.15); color: var(--critical); }
.high { background: rgba(249,115,22,0.15); color: var(--high); }
.medium { background: rgba(234,179,8,0.15); color: var(--medium); }
.low { background: rgba(34,197,94,0.15); color: var(--low); }
.finding { background: var(--bg2); border: 1px solid var(--border); border-radius: 8px; padding: 1rem; margin: 0.75rem 0; }
.finding .file { font-family: 'JetBrains Mono', monospace; font-size: 0.85rem; color: var(--accent); margin-bottom: 0.3rem; }
.finding .impact { color: var(--text-muted); font-size: 0.9rem; margin: 0.3rem 0; }
.finding .fix { color: var(--low); font-size: 0.9rem; margin-top: 0.3rem; }
.finding.cr { border-left: 3px solid var(--critical); }
.finding.hi { border-left: 3px solid var(--high); }
.finding.md { border-left: 3px solid var(--medium); }
.finding.lo { border-left: 3px solid var(--low); }
.section-grid { display: grid; gap: 0.5rem; margin: 0.5rem 0; }
.tag { display: inline-block; padding: 1px 6px; border-radius: 3px; font-size: 0.75rem; background: var(--bg2); border: 1px solid var(--border); margin-right: 4px; }
ul { padding-left: 1.5rem; margin: 0.5rem 0; }
li { margin: 0.3rem 0; }
.stats { display: grid; grid-template-columns: repeat(auto-fit, minmax(140px, 1fr)); gap: 1rem; margin: 1.5rem 0; }
.stat-card { background: var(--bg2); border: 1px solid var(--border); border-radius: 8px; padding: 1rem; text-align: center; }
.stat-card .num { font-size: 2rem; font-weight: 700; }
.stat-card .label { font-size: 0.8rem; color: var(--text-muted); margin-top: 0.2rem; }
.stat-card.crit .num { color: var(--critical); }
.stat-card.high .num { color: var(--high); }
.stat-card.med .num { color: var(--medium); }
.stat-card.low .num { color: var(--low); }
.stat-card.ok .num { color: var(--low); }
</style>
</head>
<body>

<h1>newOS4 Code Review</h1>
<p class="subtitle">Reviewed by GLM 5.2 (z-ai/glm-5.2) · v4.5.83 · 2026-07-03 · ~48K LOC across 54 modules</p>

<div class="stats">
  <div class="stat-card crit"><div class="num">2</div><div class="label">CRITICAL</div></div>
  <div class="stat-card high"><div class="num">4</div><div class="label">HIGH</div></div>
  <div class="stat-card med"><div class="num">8</div><div class="label">MEDIUM</div></div>
  <div class="stat-card low"><div class="num">5</div><div class="label">LOW</div></div>
  <div class="stat-card ok"><div class="num">19</div><div class="label">Total Issues</div></div>
</div>

<h2>1. Architecture</h2>

<h3>Strengths</h3>
<ul>
  <li><strong>Consistent module pattern</strong> — All 43 modules follow the same interface: <code>commands{}</code>, <code>registerCommands()</code>, <code>getHelpInfo()</code>, <code>init(terminal)</code></li>
  <li><strong>_safeRegister pattern</strong> (terminal.js:1216) — Uses typeof registry map instead of eval for safe class lookup</li>
  <li><strong>/sys/ virtual filesystem</strong> (virtual-fs.js:33-217) — Lazy-initialized virtual mounts with factory functions, never persisted, computed on demand</li>
  <li><strong>VFS persistence</strong> — Debounced (2s), compressed, IndexedDB fallback when localStorage quota exceeded</li>
  <li><strong>Security manager</strong> — Clean 4-level model with Levenshtein suggestion engine using Uint16Array</li>
</ul>

<h3>Weaknesses</h3>

<div class="finding md">
  <span class="severity medium">MEDIUM</span>
  <div class="file">terminal.js:13 + module-aiassist.js:1646-1661</div>
  <strong>Two divergent security whitelists</strong> — WebX11SecurityManager whitelist (~80 commands) and AI isDestructive safeRead set (~60 commands) are maintained separately. Synced in v4.5.74 but will drift as new commands are added.
</div>

<div class="finding md">
  <span class="severity medium">MEDIUM</span>
  <div class="file">module-manager.js:70, core-bundle.js:1052</div>
  <strong>new Function module loading</strong> — <code>new Function('return ' + cleanCode + '\nreturn ' + className)()</code> executes module source with full page origin access. SRI on wpm mitigates install vector but editor-saved modules and persisted reloads have no integrity check.
</div>

<div class="finding lo">
  <span class="severity low">LOW</span>
  <div class="file">module-loader.js</div>
  <strong>Procedural loader, not a class</strong> — Cannot be inspected via module-info. By design (runs before classes available) but creates inconsistency.
</div>

<div class="finding lo">
  <span class="severity low">LOW</span>
  <div class="file">terminal.js:1339</div>
  <strong>Hardcoded skipList</strong> — 21 module names hardcoded in reloadPersistedModules. New ext modules require manual list update or they double-load.
</div>

<h2>2. Security</h2>

<h3>Strengths</h3>
<ul>
  <li><strong>4-tier security model</strong> — Permissive(0)/Standard(1)/Paranoid(2)/Audit(3) with command classification into destructive/system/suspicious/unknown</li>
  <li><strong>Pipe bypass fixed</strong> (v4.5.63) — executePipeChain now calls security._classify() on each stage</li>
  <li><strong>Guardrail layer</strong> — 8 rules loaded (rm-rf-block, curl-pipe-bash, eval-danger), runs before every AI tool execution</li>
  <li><strong>SecretVault</strong> — AES-GCM + PBKDF2 at 600K iterations (OWASP 2023), sync name-only redaction (no decryption side-channel)</li>
  <li><strong>Confidence escalation</strong> — AI can auto-approve destructive tools at ≥90% confidence (configurable)</li>
</ul>

<h3>Vulnerabilities</h3>

<div class="finding cr">
  <span class="severity critical">CRITICAL</span>
  <div class="file">module-aiassist.js:1655 + console.js:31-37</div>
  <strong>AI can execute arbitrary JavaScript without confirmation</strong>
  <div class="impact"><code>node</code> is in the AI's safeRead whitelist. The <code>node -e "..."</code> command (console.js:31) uses raw <code>eval()</code> in full page context. AI can read/write localStorage (including encrypted secrets if vault unlocked), make fetch requests, modify DOM, exfiltrate data — all without user confirmation.</div>
  <div class="fix">Fix: Remove <code>node</code> from the safeRead set in <code>isDestructive()</code>. All JS execution commands should require confirmation.</div>
</div>

<div class="finding cr">
  <span class="severity critical">CRITICAL</span>
  <div class="file">module-aiassist.js:1633</div>
  <strong>shadowRollback() persistence is broken</strong>
  <div class="impact">Calls <code>this.terminal.fs.saveToLocalStorage()</code> which doesn't exist (actual method is <code>saveToStorage()</code>). VFS rollback restores in-memory state but never persists. On page reload, pre-rollback (damaged) state reappears. Silent data loss.</div>
  <div class="fix">Fix: Change <code>saveToLocalStorage()</code> → <code>saveToStorage()</code> at module-aiassist.js:1633 and core-bundle.js:9278.</div>
</div>

<div class="finding high">
  <span class="severity high">HIGH</span>
  <div class="file">terminal.js:1413-1424</div>
  <strong>capturePipeOutput lacks try/finally — print method leak on error</strong>
  <div class="impact">If <code>dispatchCommand</code> throws, <code>this.print</code> is never restored. All subsequent terminal output goes to a local variable. Terminal appears dead.</div>
  <div class="fix">Wrap in try/finally to restore <code>this.print</code>.</div>
</div>

<div class="finding high">
  <span class="severity high">HIGH</span>
  <div class="file">module-orchestrator.js:173-178</div>
  <strong>Orchestrator timers bypass central registry</strong>
  <div class="impact">3 raw <code>setInterval</code> calls (500ms A2A polling, 3s heartbeat, 30s idle sweep) not registered in <code>_timers[]</code>. <code>clearAllTimers()</code> doesn't clear them. On module unload, zombie timers continue polling VFS.</div>
  <div class="fix">Use <code>this.terminal.moduleManager._setInterval()</code> instead of raw <code>setInterval()</code>.</div>
</div>

<div class="finding high">
  <span class="severity high">HIGH</span>
  <div class="file">module-aiassist.js:1062-1066</div>
  <strong>Confidence escalation can be gamed by prompt injection</strong>
  <div class="impact">AI's own preamble text determines whether destructive tools auto-approve. If model outputs "Confidence: 95%" (genuine or injected), it bypasses human confirmation. Self-reported, not system-computed.</div>
</div>

<div class="finding medium">
  <span class="severity medium">MEDIUM</span>
  <div class="file">terminal.js:1371-1382</div>
  <strong>splitPipe doesn't distinguish quote types</strong>
  <div class="impact"><code>echo 'a|b' | cat</code> splits at the | inside single quotes because both quote types toggle the same <code>inQuote</code> boolean.</div>
  <div class="fix">Track <code>inSingle</code> and <code>inDouble</code> separately (like <code>_parseArgs</code> does).</div>
</div>

<div class="finding medium">
  <span class="severity medium">MEDIUM</span>
  <div class="file">module-aiassist.js:1379</div>
  <strong>Guardrail rules stored in localStorage, user-editable</strong>
  <div class="impact">Any code with localStorage access (including js/node commands) can modify or delete guardrail rules. Guardrail layer is bypassable by anyone who can execute JS in page context.</div>
</div>

<div class="finding medium">
  <span class="severity medium">MEDIUM</span>
  <div class="file">module-aiassist.js:1670</div>
  <strong>Guardrail check uses command-oriented regex on structured args</strong>
  <div class="impact">Guardrail regex patterns designed for command-string format (<code>/rm\s+-rf/</code>) but tool calls use structured JSON args. Pattern mismatch means some destructive tool calls may bypass guardrail rules.</div>
</div>

<h2>3. Code Quality</h2>

<div class="finding high">
  <span class="severity high">HIGH</span>
  <div class="file">core-bundle.js (16K+ lines)</div>
  <strong>Massive code duplication between source and bundle</strong>
  <div class="impact">Every fix must be applied to both the source file AND the bundle. The <code>saveToLocalStorage</code> bug exists at core-bundle.js:9278 too. Rebuild script mitigates but only if run after every change.</div>
</div>

<div class="finding medium">
  <span class="severity medium">MEDIUM</span>
  <div class="file">module-aiassist.js:914-1114</div>
  <strong>agenticLoop is a 200-line monolith</strong>
  <div class="impact">Entire tool-calling loop (loop detection, parallel safe-tool execution, destructive tool confirmation, confidence parsing, tool result logging, memory graph recording, debug panel updates) is one method. Functional but unmaintainable.</div>
</div>

<div class="finding medium">
  <span class="severity medium">MEDIUM</span>
  <div class="file">All modules</div>
  <strong>71% of localStorage writes uncovered by try/catch</strong>
  <div class="impact">162 <code>localStorage.setItem</code> calls, only 47 wrapped in try/catch. If localStorage fills up (5-10MB limit), remaining 115 calls throw unhandled QuotaExceededError.</div>
</div>

<div class="finding medium">
  <span class="severity medium">MEDIUM</span>
  <div class="file">All modules</div>
  <strong>44% of fetch calls have no timeout</strong>
  <div class="impact">105 <code>fetch()</code> calls, only 61 with AbortController/signal. A hung Ollama or SearXNG backend freezes the UI indefinitely.</div>
</div>

<div class="finding lo">
  <span class="severity low">LOW</span>
  <div class="file">terminal.js:81-97</div>
  <strong>_parseArgs doesn't handle escaped quotes</strong>
  <div class="impact"><code>\"</code> inside a double-quoted string is treated as closing quote. No backslash escaping support.</div>
</div>

<div class="finding lo">
  <span class="severity low">LOW</span>
  <div class="file">terminal.js:22</div>
  <strong>Duplicate 'cat' in internals list</strong>
  <div class="impact">'cat' appears twice in <code>_registerBuiltIn</code> array. Harmless but sloppy.</div>
</div>

<h2>4. Performance</h2>

<div class="finding medium">
  <span class="severity medium">MEDIUM</span>
  <div class="file">virtual-fs.js:425</div>
  <strong>VFS saveToStorage serializes entire filesystem</strong>
  <div class="impact"><code>JSON.stringify({ root, currentPath })</code> creates multi-MB string every 2 seconds (debounced). Synchronous, blocks main thread. IndexedDB fallback helps on overflow but serialization itself is the bottleneck.</div>
</div>

<div class="finding medium">
  <span class="severity medium">MEDIUM</span>
  <div class="file">module-orchestrator.js:173</div>
  <strong>A2A polling at 500ms</strong>
  <div class="impact">Polls VFS mailboxes every 500ms regardless of agent activity. With 5 agents, that's 10 VFS reads/second. Event bus provides instant in-memory delivery, making VFS poll redundant for same-page agents.</div>
</div>

<div class="finding medium">
  <span class="severity medium">MEDIUM</span>
  <div class="file">virtual-fs.js:55-74</div>
  <strong>vfsWalk is unbounded synchronous recursion</strong>
  <div class="impact">No depth limit. On deep directory structures, can stack overflow. /sys/vfs/stats and /sys/vfs/largest call this on every cat.</div>
</div>

<div class="finding lo">
  <span class="severity low">LOW</span>
  <div class="file">terminal.js:42-53</div>
  <strong>_collectAllCommands iterates all modules on every suggestion</strong>
  <div class="impact">Every unknown command triggers Levenshtein comparison against ~430 commands. Acceptable now but won't scale.</div>
</div>

<h2>5. AI Integration</h2>

<h3>Strengths</h3>
<ul>
  <li><strong>Well-structured agent loop</strong> — Loop detection (6x same tool+args in 30s → break), parallel safe-tool execution, sequential destructive confirmation, confidence escalation, last-turn prompt injection, memory-augmented planning</li>
  <li><strong>35+ tools</strong> — VFS operations, web search/fetch, module management, skills CRUD, VaultMind, wallet lookup, usage stats. Filtered by active loop profile.</li>
  <li><strong>6 agent loop profiles</strong> — default, coder, creative, research, safe, minimal with per-profile tool restrictions and temperature</li>
</ul>

<h3>Issues</h3>

<div class="finding medium">
  <span class="severity medium">MEDIUM</span>
  <div class="file">module-aiassist.js:44-53</div>
  <strong>AI conversation context stored in plaintext localStorage</strong>
  <div class="impact">Up to 10 context messages persisted to <code>webx11_ai_context</code>. Sensitive AI discussions persist across sessions in plaintext. No encryption, no expiry.</div>
</div>

<div class="finding lo">
  <span class="severity low">LOW</span>
  <div class="file">module-aiassist.js:1042</div>
  <strong>_summarizeObservation truncates to 150 chars</strong>
  <div class="impact">Tool results summarized for display but full result pushed to messages. Fine for display but LLM sees full results while user sees truncated ones.</div>
</div>

<h2>6. Top 5 Recommendations</h2>

<div class="finding cr">
  <span class="severity critical">#1</span>
  <div class="file">module-aiassist.js:1655</div>
  <strong>Remove node/js/js-run from AI safeRead whitelist</strong>
  <div class="impact">Immediate security hole — AI can execute arbitrary page-context JS without confirmation. One-line fix.</div>
</div>

<div class="finding cr">
  <span class="severity critical">#2</span>
  <div class="file">module-aiassist.js:1633</div>
  <strong>Fix shadowRollback() persistence</strong>
  <div class="impact">Guardrail rollback — key safety mechanism — is silently broken. <code>saveToLocalStorage()</code> → <code>saveToStorage()</code>.</div>
</div>

<div class="finding high">
  <span class="severity high">#3</span>
  <div class="file">terminal.js:1413-1424</div>
  <strong>Wrap capturePipeOutput in try/finally</strong>
  <div class="impact">Single thrown error in any pipe stage permanently breaks terminal output. Trivial fix with high impact.</div>
</div>

<div class="finding high">
  <span class="severity high">#4</span>
  <div class="file">module-orchestrator.js:173-178</div>
  <strong>Migrate orchestrator timers to central registry</strong>
  <div class="impact">3 zombie timers survive module unload. Use <code>_setInterval()</code> instead of raw <code>setInterval()</code>.</div>
</div>

<div class="finding medium">
  <span class="severity medium">#5</span>
  <div class="file">terminal.js:13 + module-aiassist.js:1646-1661</div>
  <strong>Consolidate the two security whitelists</strong>
  <div class="impact">Single <code>SecurityManager.getSafeReadCommands()</code> method eliminates drift between terminal whitelist and AI safeRead set.</div>
</div>

<hr style="border-color: var(--border); margin: 2rem 0;">
<p style="color: var(--text-muted); font-size: 0.85rem;">
  Review generated by GLM 5.2 (z-ai/glm-5.2) via Hermes Agent · newOS4 v4.5.83 · 2026-07-03
</p>

</body>
</html>