Skip to content

Commit 201e2a6

Browse files
shmuelhizmishmuel hizmiclaude
authored
Refactor echoform server, harden connections, and add wmux PWA (#22)
* Harden echoform connection safety: validation, auth, and bug fixes - Flatten plugin dirs from plugins/echoform/* to plugins/* - Update socket.io from 4.5.3 to 4.8.3, remove stale @types packages - Make callback validation reject by default (validateSchemaStrict), add skipCallbackValidation prop to opt out - Add error field to EventResponseData for client-side error handling - Add validateConnection hook to socket-server and bun-ws-server plugins - Add auth prop to socket-client, authToken to bun-ws-client - Implement per-client event authorization in singleInstance mode - Fix setMaxListeners(Infinity) → configurable limit (default 100) - Fix broken off() in socket-server (removeAllListeners → removeListener) - Fix event handler cleanup on client disconnect Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix security and code quality issues from critic review Critical bugs: - Fix error field dropped in applyBrands (decompiled-transport.ts), making client-side validation rejection actually work - Wrap validateConnection in try/catch in both socket-server and bun-ws-server to prevent process crashes - Fix stale event handlers persisting after prop deletion on view update (previously only deleteRunningView cleaned up) - Fix off("connection") no-op in socket-server by tracking wrapper references for proper removeListener matching Security: - Prevent prototype pollution in binary deserialization via Object.create(null) - Remove non-null assertion on error field in binary protocol write - Sanitize error strings sent to client (no stack traces) - Make ValidationResult a discriminated union (illegal states impossible) Code quality: - Extract authorizeEventUidsForAllClients/deauthorizeEventUidsForAllClients helpers (DRY) - Extract removeEventHandlers helper - Extract buildAuthenticatedUrl from bun-ws-client - Replace 0xFF magic number with EVENT_FALLBACK constant - Rename abbreviated variables (p→prop, v→view) - Remove section divider comments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix listener leaks, missing cleanup, and token rotation bugs - Move connectionWrappers Map to useRef in socket-server so wrapper references persist across renders (fixes off() being a no-op) - Fire disconnect handlers in bun-ws-server stop() before clearing client map (fixes missed application cleanup on shutdown) - Add options?.authToken to useEffect deps in bun-ws-client so token rotation triggers reconnection - Add cleanup function to Server.tsx useEffect that calls transport.off on connection handler (fixes listener leak on transport/prop changes) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix 24 code quality and security issues from round 3 critic review Security: add depth limit and collection length cap to binary protocol deserialization, narrow try/catch scope in respondToEventCodec, sanitize validation errors sent to clients, fix error leaking in socket.io validateConnection, add auth to socket-client useEffect deps. Code quality: refactor App.tsx into small (<20 line) functions, eliminate 5x duplicated respond_to_event emit pattern, make auth map operations immutable (return new Maps/Sets), flatten nested ifs in bun-ws-server and socket-server, extract DRY validation error helper, fix asymmetric off handler, rename vague variables. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Simplify binary-protocol.ts using typed-binary high-level API Replace hand-rolled view/event codecs with typed-binary's object(), dynamicArrayOf(), and concat() schemas. Eliminates 9 view helper functions, 6 EventCodec objects, the EventCodec interface, and 3 dispatch functions. Wire format is byte-compatible (verified by round-trip tests). Custom codecs remain only where necessary: recursive SerializableValue (depth guards), prop schema (wire order constraint), and respond_to_event (backward-compat try/catch). 604 lines → 349 lines (42% reduction). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add PWA support and remove TabBar in favor of sidebar-only navigation - Add PWA manifest, service worker, and SVG icons for installability - Add capture-phase keyboard handler for PWA shortcuts (Cmd+T, Cmd+P, Ctrl+Tab) - Remove TabBar component; all navigation now via sidebar - Add inline process start/stop/restart buttons on sidebar tab items - Add PWA install banner in sidebar footer - Detect standalone mode for additional shortcut hints Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Refactor echoform server, binary protocol, and plugin improvements - Extract ViewReconcileContext interface to reduce parameter passing - Export AnyTransport from shared types - Add depth tracking to binary protocol write/measure for nesting safety - Fix unauthorized event access check (reject when auth set missing) - Refactor decompiled-transport: discriminated union for UnbrandedProp, direct binary emit - Add corsOrigin option to bun-ws-server - Remove unused type parameters in socket-client - Refactor socket-server to use useMemo for transport, inline ServerBase props Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat/connection-safety-hardening * Make view() return usable React components, eliminate useViews hook view() now returns an object that is both a ViewDef (for type inference via InferClientProps/InferServerProps) and a React.FC (for direct JSX usage on the server). Components read from a ViewFactoryContext provided by <App>, keeping server rendering logic out of the shared module. This removes useViews entirely — server code imports view definitions directly and uses them as JSX components with zero setup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix listener cleanup in Server and App effects Add proper disconnect handler tracking and cleanup in Server.tsx, and fix registerSocketListener effect to return cleanup function. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix readonly name property error in view() builder Object.assign(component, def) threw because function.name is non-writable. Exclude name from the assign since it's already set via Object.defineProperty. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix dev-server E2E tests for multi-terminal UI Use .first() on .xterm locators since the sidebar layout now renders multiple terminal instances simultaneously. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix initial tab visibility and update dev-server E2E tests Initialize activeTabId from the first category's first tab instead of empty string, so the active tab is visible on load. Update E2E tests to use visibility-aware selectors and sidebar-based navigation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: shmuel hizmi <shmuel@tov.dev> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent d523ebf commit 201e2a6

61 files changed

Lines changed: 1511 additions & 1355 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
---
2+
name: critic-review
3+
description: "Spawn 6 critic agents (3 code quality, 3 security) to review recent changes, validate all claims, and present findings. Triggers on: critic review, review critics, spawn critics, code review agents, security review agents."
4+
---
5+
6+
# Critic Review
7+
8+
Spawn parallel critic agents to review code changes, validate their findings, and present actionable results.
9+
10+
---
11+
12+
## The Job
13+
14+
1. **Identify changed files** from the current branch vs base
15+
2. **Spawn 6 critic agents** in parallel (3 code quality, 3 security)
16+
3. **Collect and deduplicate** all findings
17+
4. **Validate each claim** by reading the actual code
18+
5. **Present validated findings** grouped by severity
19+
20+
---
21+
22+
## Step 1: Identify Changed Files
23+
24+
Determine what files to review:
25+
26+
```bash
27+
# Get the diff of changed files
28+
git diff --name-only HEAD~$(git rev-list --count HEAD --not $(git merge-base HEAD master))..HEAD 2>/dev/null || git diff --name-only HEAD~3..HEAD
29+
```
30+
31+
If the user specifies particular files or directories, scope the review to those instead.
32+
33+
Read all changed files so you can provide them as context to the critic agents.
34+
35+
---
36+
37+
## Step 2: Spawn Critic Agents
38+
39+
Launch **6 agents in parallel** using the Agent tool. All agents should receive:
40+
- The full content of each changed file
41+
- The diff showing what changed
42+
43+
### Code Quality Critics (3 agents)
44+
45+
Each code quality critic should focus on a different area. Read the project's coding rules from `docs/coding-rules.md` and include them in the agent prompt.
46+
47+
**Agent 1 — Structure & Patterns:**
48+
- Zero mutation violations
49+
- Nested if statements (must be zero)
50+
- Function size (≤20 lines)
51+
- Single responsibility principle
52+
- Classes (must not exist — functional only)
53+
54+
**Agent 2 — Types & Naming:**
55+
- `any` type usage
56+
- Descriptive variable/function names
57+
- Strict interfaces
58+
- Reuse of existing types
59+
- Boolean function naming (is/has/can/should prefixes)
60+
61+
**Agent 3 — Code Hygiene:**
62+
- DRY violations
63+
- Comments explaining "what" instead of "why"
64+
- Error handling patterns
65+
- Dead code
66+
- Dependency inversion
67+
68+
### Security Critics (3 agents)
69+
70+
**Agent 4 — Input Validation & Injection:**
71+
- Input validation at system boundaries
72+
- Prototype pollution vectors
73+
- Injection risks (command, XSS, etc.)
74+
- Unsafe type assertions that skip validation
75+
76+
**Agent 5 — Auth & Access Control:**
77+
- Authentication bypass vectors
78+
- Authorization check completeness
79+
- Token/credential handling
80+
- Session management
81+
- Event authorization gaps
82+
83+
**Agent 6 — Transport & Protocol Security:**
84+
- WebSocket security (origin checks, message validation)
85+
- Binary protocol parsing safety
86+
- Error message information leakage
87+
- CORS configuration
88+
- Race conditions in connection/disconnection handling
89+
90+
### Agent Prompt Template
91+
92+
Each agent prompt MUST include:
93+
94+
```
95+
You are a critic reviewing code changes. Your job is to find REAL issues, not theoretical ones.
96+
97+
**Rules:**
98+
- Only report issues in the CHANGED code (not pre-existing issues)
99+
- Each finding must reference a specific file and line number
100+
- Each finding must include the actual problematic code snippet
101+
- Rate severity: HIGH (exploitable/broken), MEDIUM (should fix), LOW (nice to have)
102+
- Do NOT report issues that are by design or documented trade-offs
103+
- Be specific — "could be improved" is not a finding
104+
105+
**Output format for each finding:**
106+
### [SEVERITY] Title
107+
- **File:** path/to/file.ts:LINE
108+
- **Code:** `the problematic code`
109+
- **Issue:** What's wrong
110+
- **Fix:** How to fix it
111+
```
112+
113+
---
114+
115+
## Step 3: Collect and Deduplicate
116+
117+
After all 6 agents complete:
118+
119+
1. Gather all findings into a single list
120+
2. Remove exact duplicates (same file, same line, same issue)
121+
3. Merge similar findings that point to the same root cause
122+
4. Sort by severity: HIGH → MEDIUM → LOW
123+
124+
---
125+
126+
## Step 4: Validate Each Claim
127+
128+
For every finding, **read the actual code** at the referenced location and verify:
129+
130+
1. Does the file and line exist?
131+
2. Does the code snippet match what's actually there?
132+
3. Is the issue real or a false positive?
133+
4. Is this a change we made, or pre-existing code?
134+
135+
Mark each finding as:
136+
- **CONFIRMED** — Issue is real and in changed code
137+
- **FALSE POSITIVE** — Code is correct, critic was wrong (explain why)
138+
- **PRE-EXISTING** — Real issue but not in our changes
139+
- **BY DESIGN** — Intentional trade-off (explain rationale)
140+
141+
---
142+
143+
## Step 5: Present Results
144+
145+
Show the user a summary table:
146+
147+
```
148+
## Critic Review Results
149+
150+
### Summary
151+
- Total findings: X
152+
- Confirmed: X (Y high, Z medium, W low)
153+
- False positives: X
154+
- Pre-existing: X
155+
- By design: X
156+
157+
### Confirmed Findings
158+
159+
#### HIGH
160+
[List each confirmed HIGH finding with file, line, issue, and suggested fix]
161+
162+
#### MEDIUM
163+
[List each confirmed MEDIUM finding]
164+
165+
#### LOW
166+
[List each confirmed LOW finding]
167+
168+
### Dismissed Findings
169+
[Brief list of false positives / pre-existing / by-design with one-line explanations]
170+
```
171+
172+
---
173+
174+
## Important Notes
175+
176+
- **Do NOT fix anything** — this skill only reports. The user decides what to fix.
177+
- **Validate before presenting** — never show unvalidated claims to the user.
178+
- **Scope to changes** — pre-existing issues should be noted but clearly labeled.
179+
- If a critic agent fails or returns no results, note it in the summary.
180+
- Use `subagent_type: "general-purpose"` for all critic agents.

README.md

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,11 @@ export const views = createViews({ Dashboard, ProcessTable, LogStream });
5858
// server/index.tsx
5959
import os from "os";
6060
import { Render } from "@playfast/echoform-render";
61-
import { Server, useViews, useStream } from "@playfast/echoform/server";
61+
import { Server, useStream } from "@playfast/echoform/server";
6262
import { createBunWebSocketServer } from "@playfast/echoform-bun-ws-server";
63-
import { views, LogStream } from "../shared/views";
63+
import { Dashboard, ProcessTable, LogStream } from "../shared/views";
6464

6565
function Monitor() {
66-
const View = useViews(views);
6766
const log = useStream(LogStream, "lines");
6867
const [processes, setProcesses] = useState([]);
6968

@@ -76,25 +75,23 @@ function Monitor() {
7675
return () => clearInterval(interval);
7776
}, []);
7877

79-
if (!View) return null;
80-
8178
return (
8279
<>
83-
<View.Dashboard
80+
<Dashboard
8481
hostname={os.hostname()}
8582
cpuUsage={getCpuUsage()}
8683
memoryUsed={os.totalmem() - os.freemem()}
8784
memoryTotal={os.totalmem()}
8885
/>
89-
<View.ProcessTable
86+
<ProcessTable
9087
processes={processes}
9188
onKill={(pid) => {
9289
process.kill(pid, "SIGTERM");
9390
log.emit(`Killed PID ${pid}`);
9491
}}
9592
onRefresh={() => refresh()}
9693
/>
97-
<View.LogStream lines={log} />
94+
<LogStream lines={log} />
9895
</>
9996
);
10097
}
@@ -152,16 +149,15 @@ The server reads system data, manages state, handles kill signals. The client is
152149

153150
| Function | Description |
154151
|----------|-------------|
155-
| `view(name, config)` | Define a view with input, callbacks, and streams |
152+
| `view(name, config)` | Define a view — returns a React component usable as JSX on the server |
156153
| `callback(config?)` | Define a callback with optional input/output schemas |
157154
| `stream(schema)` | Define a server→client stream with chunk schema |
158-
| `createViews(record)` | Compose view definitions into a registry |
155+
| `createViews(record)` | Compose view definitions into a registry (optional, for validation) |
159156

160157
### Server (`@playfast/echoform/server`)
161158

162159
| Export | Description |
163160
|--------|-------------|
164-
| `useViews(viewDefs)` | Get typed view components for rendering |
165161
| `useStream(viewDef, name)` | Create a `StreamEmitter` for pushing data to clients |
166162
| `Server` | Root component that manages client connections |
167163

0 commit comments

Comments
 (0)