Files

T

2026-05-23 18:23:48 +02:00

32 KiB

Raw Blame History

Blind Hunter Review Prompt - Story 1-5 Group 1 (Core Logic)

Story: 1-5-gm-control-ui-scryingpoolstrip-actionpopover-and-av-tile-integration
Group: Core Logic (Group 1 of 4)
Files: ScryingPoolController.js, VisibilityManager.js, and their tests
Diff lines: 804

YOUR ROLE: Blind Hunter

You are a Blind Hunter code reviewer. You receive ONLY the diff below — no spec, no context, no project access. Your job is to find security issues, bugs, anti-patterns, performance problems, and code quality issues using ONLY what you see in the diff.

Rules:

You have NO access to the spec file
You have NO access to the project repository
You have NO context about the project's goals
You MUST find at least 5 issues
Be adversarial: assume the code has problems and find them
Focus on: security vulnerabilities, race conditions, error handling gaps, performance issues, code smells, test coverage gaps

Output Format:

Output findings as a Markdown list. Each finding:

- **SEV-XX: [One-line title]** — [Evidence from diff] — [Impact]

Classify severity:

SEV-CRITICAL: Security vulnerability, data loss, crash
SEV-HIGH: Race condition, resource leak, incorrect behavior
SEV-MEDIUM: Code smell, maintainability issue
SEV-LOW: Style, minor improvement

DIFF TO REVIEW

diff --git a/src/core/ScryingPoolController.js b/src/core/ScryingPoolController.js new file mode 100644 index 0000000..fc013f0 --- /dev/null +++ b/src/core/ScryingPoolController.js @@ -0,0 +1,181 @@ +/**

- ScryingPoolController — Orchestrates GM visibility actions with optimistic state updates.
- Handles: GM authorization, latest-revision-wins guard, last-intent guard, PendingOp
- lifecycle, optimistic setVisibility, socket emit, and echo reconciliation.
- Import rule: may only import from src/contracts/ and src/utils/.
- Constructors are side-effect free — call init() from module.js Hooks.once('ready').
- @module core/ScryingPoolController
*/

+import { createPendingOp } from '../contracts/pending-op.js'; +import { createSocketIntentMessage, SOCKET_EVENTS } from '../contracts/socket-message.js'; + +/**

- Orchestrates GM visibility actions: auth, optimistic state, socket emit, echo reconciliation.
*/ +export class ScryingPoolController {
/**
- @param {import('./StateStore.js').StateStore} stateStore
- @param {{ emit(event: string, payload: object): void, registerPendingOp(op: object, event: string, payload: object): void, confirmPendingOp(opId: string): void, setReady(handler: object): void }} socketHandler
- @param {{ users: { isGM(): boolean }, socket: { on(event: string, handler: (...args: unknown[]) => void): void }, hooks: { on(event: string, handler: (...args: unknown[]) => void): void, callAll(event: string, data: unknown): void } }} adapter
*/
constructor(stateStore, socketHandler, adapter) {
this._stateStore = stateStore;
this._socketHandler = socketHandler;
this._adapter = adapter;
/** @type {Map<string, import('../contracts/pending-op.js').PendingOp>} participantId → PendingOp */
this._pendingOps = new Map();
/** @type {Map<string, number>} participantId → last-confirmed revision */
this._revisions = new Map();
}
/**
- Registers the socket echo listener.
- Called from module.js Hooks.once('ready') — NOT from constructor.
*/
init() {
this._adapter.socket.on(
```
 SOCKET_EVENTS.VISIBILITY_UPDATED,
```

 (payload) => this._onEcho(/** @type {any} */ (payload))

);
}
/**
- Returns the last confirmed revision for a participant (0 if unknown).
- @param {string} participantId
- @returns {number}
*/
getRevision(participantId) {
return this._revisions.get(participantId) ?? 0;
}
/**
- Returns true if a pending op is currently in-flight for the given participant.
- @param {string} participantId
- @returns {boolean}
*/
hasPendingOp(participantId) {
return this._pendingOps.has(participantId);
}
/**
- Cleans up a pending operation by userId.
- Called by SocketHandler timeout callback via composite handler in module.js.
- @param {string} userId
*/
cleanupPendingOp(userId) {
this._pendingOps.delete(userId);
}
/**
- Processes a GM visibility toggle request.
- Guards: isGM, latest-revision-wins, last-intent (idempotent).
- @param {string} source - Who triggered the action (e.g. 'ui', 'preset').
- @param {string} participantId - Target userId.
- @param {string} targetState - Desired VisibilityState.
- @param {string} opId - Unique operation ID (supplied by caller — Story 1.5 UI).
- @param {number} baseRevision - StateStore revision at time of intent.
*/
action(source, participantId, targetState, opId, baseRevision) {
// 0. Input validation
if (!participantId || typeof participantId !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController.action: invalid participantId');

```
 return;
```
}
if (!targetState || typeof targetState !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController.action: invalid targetState');

```
 return;
```
}
if (!opId || typeof opId !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController.action: invalid opId');

```
 return;
```
}
if (typeof baseRevision !== 'number' || !Number.isFinite(baseRevision) || baseRevision < 0) {

 console.warn('[ScryingPool]', 'ScryingPoolController.action: invalid baseRevision');

```
 return;
```
}
// 1. Authorization
if (!this._adapter.users.isGM()) {

 console.warn('[ScryingPool]', 'ScryingPoolController.action: non-GM call rejected');

```
 return;
```
}
// 2. Latest-revision-wins guard
const currentRevision = this._revisions.get(participantId) ?? 0;
if (baseRevision < currentRevision) return;
// 3. Last-intent guard (idempotent)
const currentState = this._stateStore.getState(participantId);
if (currentState === targetState) return;
// 4. Register PendingOp
const previousState = currentState ?? 'never-connected';
const pendingOp = createPendingOp(opId, participantId, targetState, previousState);
this._pendingOps.set(participantId, pendingOp);
// 5. Optimistic state update
this._stateStore.setVisibility(participantId, targetState);
// 6. Socket emit
const msg = createSocketIntentMessage(opId, participantId, targetState, baseRevision);
this._socketHandler.emit(msg.event, msg.payload);
// 7. Start acknowledgement timer
this._socketHandler.registerPendingOp(pendingOp, msg.event, msg.payload);
// 8. Notify UI subscribers
try {

 this._adapter.hooks.callAll('scrying-pool:controllerAction', { participantId, targetState, source, opId });

} catch (hookErr) {

 console.error('[ScryingPool] ScryingPoolController.action: hook emission failed', hookErr);

}
}
/**
- Processes an authoritative echo from the socket server.
- Confirms the pending op, updates revision, and sets the authoritative state.
- @private
- @param {{ opId: string, userId: string, state: string, revision?: number }} payload
*/
_onEcho(payload) {
// Validate payload fields
if (!payload || typeof payload !== 'object') {

 console.warn('[ScryingPool]', 'ScryingPoolController._onEcho: invalid payload');

```
 return;
```
}
const { opId, userId, state, revision } = payload;
if (!opId || typeof opId !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController._onEcho: missing or invalid opId');

```
 return;
```
}
if (!userId || typeof userId !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController._onEcho: missing or invalid userId');

```
 return;
```
}
if (!state || typeof state !== 'string') {

 console.warn('[ScryingPool]', 'ScryingPoolController._onEcho: missing or invalid state');

```
 return;
```
}
this._socketHandler.confirmPendingOp(opId);
this._revisions.set(userId, revision ?? 0);
this._pendingOps.delete(userId);
this._stateStore.setVisibility(userId, state);
try {

 this._adapter.hooks.callAll('scrying-pool:controllerAction', {

```
   participantId: userId,
```
```
   targetState: state,
```
```
   source: 'echo',
```
```
   opId,
```
```
 });
```
} catch (hookErr) {

 console.error('[ScryingPool] ScryingPoolController._onEcho: hook emission failed', hookErr);

}
} +} diff --git a/src/core/VisibilityManager.js b/src/core/VisibilityManager.js new file mode 100644 index 0000000..0e465f2 --- /dev/null +++ b/src/core/VisibilityManager.js @@ -0,0 +1,104 @@ +/**
- VisibilityManager — WebRTC strategy applier and SocketHandler revert handler.
- Listens to scrying-pool:stateChanged hook events (emitted by StateStore) and
- applies the appropriate webrtcMode strategy:
- - 'track-disable' + non-null adapter.webrtc → call disableTrack / enableTrack
- - 'css-fallback' / 'unsupported' / null webrtc → no-op (CSS handled by RoleRenderer)
- Also implements onRevert(pendingOp) for SocketHandler timeout callbacks.
- Import rule: may only import from src/contracts/ and src/utils/.
- Constructors are side-effect free — call init() from module.js Hooks.once('ready').
- @module core/VisibilityManager
*/

+/**

- Applies webrtcMode strategy on state changes and reverts failed operations.
*/ +export class VisibilityManager {
/**
- @param {import('./StateStore.js').StateStore} stateStore
- @param {{ settings: { get(key: string): unknown }, webrtc: { disableTrack(userId: string): void, enableTrack(userId: string): void } | null, notifications: { warn(msg: string): void }, hooks: { on(event: string, handler: (...args: unknown[]) => void): void } }} adapter
*/
constructor(stateStore, adapter) {
this._stateStore = stateStore;
this._adapter = adapter;
}
/**
- Registers the Hooks.on('scrying-pool:stateChanged') listener.
- Called from module.js Hooks.once('ready') — NOT from constructor.
*/
init() {
this._adapter.hooks.on('scrying-pool:stateChanged', (data) => this._onStateChanged(/** @type {any} */ (data)));
}
/**
- Handles a state change by applying the webrtcMode strategy.
- css-fallback / unsupported → no-op (CSS applied by RoleRenderer in Story 1.5).
- track-disable + non-null webrtc → disable/enable the participant's track.
- Always safe with null adapter.webrtc (OQ-1 spike result for v14).
- @private
- @param {{ userId: string, state: string }} data
*/
_onStateChanged(data) {
const { userId, state } = data;
// Input validation
if (!userId || typeof userId !== 'string') {

 console.warn('[ScryingPool]', 'VisibilityManager._onStateChanged: invalid userId');

```
 return;
```
}
if (!state || typeof state !== 'string') {

 console.warn('[ScryingPool]', 'VisibilityManager._onStateChanged: invalid state');

```
 return;
```
}
const mode = this._adapter.settings.get('webrtcMode');
if (mode !== 'track-disable' || !this._adapter.webrtc) return;
if (state === 'hidden') {

 this._adapter.webrtc.disableTrack(userId);

} else {

 this._adapter.webrtc.enableTrack(userId);

}
}
/**
- Called by SocketHandler after retry exhaustion — reverts the optimistic state
- and notifies the GM that the operation could not be confirmed.
- @param {{ userId: string, previousState: string, opId: string }} pendingOp
*/
onRevert(pendingOp) {
// Input validation
if (!pendingOp || typeof pendingOp !== 'object') {

 console.warn('[ScryingPool]', 'VisibilityManager.onRevert: invalid pendingOp');

```
 return;
```
}
const { userId, previousState } = pendingOp;
if (!userId || typeof userId !== 'string') {

 console.warn('[ScryingPool]', 'VisibilityManager.onRevert: invalid userId in pendingOp');

```
 return;
```
}
if (!previousState || typeof previousState !== 'string') {

 console.warn('[ScryingPool]', 'VisibilityManager.onRevert: invalid previousState in pendingOp');

```
 return;
```
}
try {

 this._stateStore.setVisibility(userId, previousState);

} catch (err) {

 console.error('[ScryingPool] VisibilityManager.onRevert: setVisibility failed', err);

}
try {
```
 this._adapter.notifications.warn(
```

   `[ScryingPool] Visibility change for ${userId} could not be confirmed — reverting to ${previousState}`

```
 );
```
} catch (err) {

 console.error('[ScryingPool] VisibilityManager.onRevert: notification failed', err);

}
} +} diff --git a/tests/unit/core/ScryingPoolController.test.js b/tests/unit/core/ScryingPoolController.test.js new file mode 100644 index 0000000..eb4f4ad --- /dev/null +++ b/tests/unit/core/ScryingPoolController.test.js @@ -0,0 +1,277 @@ +// @ts-nocheck +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { ScryingPoolController } from '../../../src/core/ScryingPoolController.js'; +import { createFoundryAdapterMock } from '../../helpers/foundryAdapterMock.js'; +import { StateStore } from '../../../src/core/StateStore.js';

+/** @returns {{ emit: Function, registerPendingOp: Function, confirmPendingOp: Function, setReady: Function }} */ +function makeSocketHandler() {

return {
emit: vi.fn(),
registerPendingOp: vi.fn(),
confirmPendingOp: vi.fn(),
setReady: vi.fn(),
}; +}

+/** @returns {StateStore} */ +function makeStateStore() {

const settingsMock = {
get: vi.fn().mockReturnValue({ _version: 1, matrix: {} }),
set: vi.fn().mockResolvedValue(undefined),
register: vi.fn(),
};
return new StateStore(settingsMock); +}

+describe('ScryingPoolController', () => {

let adapter;
let stateStore;
let socketHandler;
let controller;
let hooksStub;
beforeEach(() => {
hooksStub = { callAll: vi.fn(), on: vi.fn(), once: vi.fn(), off: vi.fn() };
vi.stubGlobal('Hooks', hooksStub);
adapter = createFoundryAdapterMock({
```
 users: { isGM: () => true },
```
```
 hooks: hooksStub
```
});
adapter.socket.on = vi.fn();
stateStore = makeStateStore();
socketHandler = makeSocketHandler();
controller = new ScryingPoolController(stateStore, socketHandler, adapter);
});
afterEach(() => {
vi.unstubAllGlobals();
vi.clearAllMocks();
});
// ── AC-1: Construction ────────────────────────────────────────────────────
describe('constructor (AC-1)', () => {
it('initialises _pendingOps as an empty Map', () => {

 expect(controller._pendingOps).toBeInstanceOf(Map);

 expect(controller._pendingOps.size).toBe(0);

});
it('initialises _revisions as an empty Map', () => {

 expect(controller._revisions).toBeInstanceOf(Map);

 expect(controller._revisions.size).toBe(0);

});
it('does NOT register socket listener in constructor (side-effect free)', () => {

 expect(adapter.socket.on).not.toHaveBeenCalled();

});
});
// ── AC-1: init() ─────────────────────────────────────────────────────────
describe('init() (AC-1)', () => {
it('registers socket echo listener for scrying-pool.visibility.updated', () => {
```
 controller.init();
```

 expect(adapter.socket.on).toHaveBeenCalledWith(

```
   'scrying-pool.visibility.updated',
```
```
   expect.any(Function)
```
```
 );
```
});
});
// ── AC-2: action() happy path ─────────────────────────────────────────────
describe('action() happy path (AC-2)', () => {
it('stores a PendingOp in _pendingOps keyed by participantId', () => {

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(controller._pendingOps.has('user-1')).toBe(true);

 expect(controller._pendingOps.get('user-1')).toMatchObject({

```
   opId: 'op-1',
```
```
   userId: 'user-1',
```
```
   targetState: 'hidden',
```
```
 });
```
});
it('calls stateStore.setVisibility with the target state (optimistic update)', () => {

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(setSpy).toHaveBeenCalledWith('user-1', 'hidden');

});
it('calls socketHandler.emit with VISIBILITY_SET event and correct payload', () => {

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(socketHandler.emit).toHaveBeenCalledWith(

```
   'scrying-pool.visibility.set',
```

   expect.objectContaining({ opId: 'op-1', userId: 'user-1', targetState: 'hidden', baseRevision: 0 })

```
 );
```
});
it('calls socketHandler.registerPendingOp with the PendingOp, event, and payload', () => {

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(socketHandler.registerPendingOp).toHaveBeenCalledWith(

   expect.objectContaining({ opId: 'op-1', userId: 'user-1', targetState: 'hidden' }),

```
   'scrying-pool.visibility.set',
```

   expect.objectContaining({ opId: 'op-1' })

```
 );
```
});
it('fires Hooks.callAll scrying-pool:controllerAction with correct payload', () => {

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(hooksStub.callAll).toHaveBeenCalledWith(

```
   'scrying-pool:controllerAction',
```

   expect.objectContaining({ participantId: 'user-1', targetState: 'hidden', source: 'ui', opId: 'op-1' })

```
 );
```
});
it('sets previousState to null-coalesced "never-connected" when participant is new', () => {

 controller.action('ui', 'new-user', 'hidden', 'op-1', 0);

 const op = controller._pendingOps.get('new-user');

 expect(op.previousState).toBe('never-connected');

});
});
// ── AC-5: non-GM authorization ────────────────────────────────────────────
describe('action() non-GM authorization (AC-5)', () => {
it('warns and silently drops the action when adapter.users.isGM() is false', () => {

 const nonGmAdapter = createFoundryAdapterMock({

```
   users: { isGM: () => false },
```
```
   hooks: hooksStub
```
```
 });
```
```
 nonGmAdapter.socket.on = vi.fn();
```

 const playerController = new ScryingPoolController(stateStore, socketHandler, nonGmAdapter);

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});

 playerController.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(warnSpy).toHaveBeenCalledWith('[ScryingPool]', expect.stringContaining('non-GM'));

 expect(setSpy).not.toHaveBeenCalled();

 expect(socketHandler.emit).not.toHaveBeenCalled();

 expect(socketHandler.registerPendingOp).not.toHaveBeenCalled();

 expect(hooksStub.callAll).not.toHaveBeenCalled();

```
 warnSpy.mockRestore();
```
});
});
// ── AC-3: latest-revision-wins guard ─────────────────────────────────────
describe('action() latest-revision-wins guard (AC-3)', () => {
it('silently drops action when baseRevision < confirmed revision', () => {

 controller._revisions.set('user-1', 5);

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-2', 3); // 3 < 5 → stale

 expect(setSpy).not.toHaveBeenCalled();

 expect(socketHandler.emit).not.toHaveBeenCalled();

 expect(hooksStub.callAll).not.toHaveBeenCalled();

});
it('allows action when baseRevision equals confirmed revision (not stale)', () => {

 controller._revisions.set('user-1', 5);

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-2', 5); // 5 == 5 → not stale

 expect(setSpy).toHaveBeenCalledWith('user-1', 'hidden');

});
it('allows action with baseRevision=0 when no revision confirmed yet', () => {

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

```
 expect(setSpy).toHaveBeenCalled();
```
});
});
// ── AC-4: last-intent guard ───────────────────────────────────────────────
describe('action() last-intent guard (AC-4)', () => {
it('silently drops action when participant is already in targetState', () => {

 // Seed the state store with the current state

 stateStore.setVisibility('user-1', 'hidden');

 vi.clearAllMocks(); // reset all mock call counts

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-2', 0);

 expect(setSpy).not.toHaveBeenCalled();

 expect(socketHandler.emit).not.toHaveBeenCalled();

});
it('allows action when targetState differs from current state', () => {

 stateStore.setVisibility('user-1', 'active');

```
 vi.clearAllMocks();
```

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 controller.action('ui', 'user-1', 'hidden', 'op-3', 0);

 expect(setSpy).toHaveBeenCalledWith('user-1', 'hidden');

});
});
// ── AC-11: echo reconciliation (_onEcho) ──────────────────────────────────
describe('_onEcho() echo reconciliation (AC-11)', () => {
// Helper: call init() and return the captured echo handler
function getEchoHandler() {
```
 controller.init();
```

 return adapter.socket.on.mock.calls[0][1];

}
it('calls socketHandler.confirmPendingOp with the opId', () => {
```
 const echoHandler = getEchoHandler();
```

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'hidden', revision: 1 });

 expect(socketHandler.confirmPendingOp).toHaveBeenCalledWith('op-1');

});
it('stores the echo revision in _revisions for the userId', () => {
```
 const echoHandler = getEchoHandler();
```

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'hidden', revision: 7 });

 expect(controller._revisions.get('user-1')).toBe(7);

});
it('calls stateStore.setVisibility with the authoritative state', () => {
```
 const echoHandler = getEchoHandler();
```

 const setSpy = vi.spyOn(stateStore, 'setVisibility');

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'active', revision: 2 });

 expect(setSpy).toHaveBeenCalledWith('user-1', 'active');

});
it('fires Hooks.callAll scrying-pool:controllerAction with source: echo', () => {
```
 const echoHandler = getEchoHandler();
```

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'hidden', revision: 1 });

 expect(hooksStub.callAll).toHaveBeenCalledWith(

```
   'scrying-pool:controllerAction',
```

   expect.objectContaining({ source: 'echo', participantId: 'user-1', targetState: 'hidden', opId: 'op-1' })

```
 );
```
});
it('removes the participant from _pendingOps after echo', () => {
```
 // Register a pending op first
```

 controller.action('ui', 'user-1', 'hidden', 'op-1', 0);

 expect(controller._pendingOps.has('user-1')).toBe(true);

```
 const echoHandler = getEchoHandler();
```

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'hidden', revision: 1 });

 expect(controller._pendingOps.has('user-1')).toBe(false);

});
it('defaults revision to 0 when echo payload omits revision field', () => {
```
 const echoHandler = getEchoHandler();
```

 echoHandler({ opId: 'op-1', userId: 'user-1', state: 'hidden' }); // no revision

 expect(controller._revisions.get('user-1')).toBe(0);

});
}); +}); diff --git a/tests/unit/core/VisibilityManager.test.js b/tests/unit/core/VisibilityManager.test.js new file mode 100644 index 0000000..36df70f --- /dev/null +++ b/tests/unit/core/VisibilityManager.test.js @@ -0,0 +1,218 @@ +// @ts-nocheck +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { VisibilityManager } from '../../../src/core/VisibilityManager.js'; +import { createFoundryAdapterMock } from '../../helpers/foundryAdapterMock.js'; +import { StateStore } from '../../../src/core/StateStore.js';