test(mock-api): add fuzzy socket-aware harness#1963
Merged
senamakel merged 6 commits intoMay 16, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughAdds stateful in-memory mocks (conversations, messages, cron, webhooks, LLM threads), a full Socket.IO mock server (polling + WebSocket), deterministic fuzzing helpers, global request behavior (jittered delays and HTTP fault injection), admin socket controls, expanded LLM dynamic responses, and tests + test helpers. ChangesMock API Stateful Infrastructure
Sequence Diagram(s) sequenceDiagram
participant Client
participant MockServer
participant SocketCore
participant MockState
Client->>MockServer: HTTP /socket.io GET/POST (polling) or WS upgrade
MockServer->>SocketCore: handleSocketRequest(ctx) / handleWebSocketUpgrade(req,socket)
SocketCore->>MockState: registerSocketSession / getSocketSession / queue/drain packets
SocketCore-->>Client: engine-open, connect ack, ready, or error packets
MockServer->>MockState: maybeApplyGlobalBehavior checks httpFaultRules / apply delay
MockState-->>MockServer: state responses (conversations, events, sessions)
MockServer-->>Client: HTTP route responses or injected fault JSON
Estimated code review effort: Possibly related PRs:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/mock-api/routes/cron.mjs (1)
83-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelete response reports success even when nothing was removed.
deletedis derived from the HTTP method, not from whether a matching record existed.Suggested fix
- json(res, 200, { - success: true, - data: { id: cronItem[1], deleted: method === "DELETE" }, - }); + json(res, 200, { + success: true, + data: { id: cronItem[1], deleted: method === "DELETE" && index >= 0 }, + });- json(res, 200, { - success: true, - data: { id: trgItem[1], deleted: method === "DELETE" }, - }); + json(res, 200, { + success: true, + data: { id: trgItem[1], deleted: method === "DELETE" && index >= 0 }, + });Also applies to: 124-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mock-api/routes/cron.mjs` around lines 83 - 89, The response currently sets deleted based on the HTTP method instead of whether a matching record was actually removed; update the logic in the handler around cronJobs.splice and the json(...) response so deleted is true only when a record was found and removed (e.g., use index >= 0 or a removed flag set when cronJobs.splice runs) and return the actual id from cronItem when present; apply the same fix to the duplicate block referencing cronJobs.splice / cronItem around the later section (lines 124-130) so both endpoints report deletion status correctly.
🧹 Nitpick comments (2)
scripts/mock-api/state.mjs (1)
104-109: ⚡ Quick winMake
createMockId()seed-stable.
Date.now()andMath.random()make IDs drift across identical seeds, which undercuts reproducible mock fixtures and event logs. Derive the suffix frombehaviorSeed()plus the sequence only.Suggested change
export function createMockId(prefix = "mock") { - return `${prefix}_${nextMockSequence()}_${hashString( - `${prefix}:${behaviorSeed()}:${Date.now()}:${Math.random()}`, - ) + const sequence = nextMockSequence(); + return `${prefix}_${sequence}_${hashString( + `${prefix}:${behaviorSeed()}:${sequence}`, + ) .toString(16) .slice(0, 8)}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mock-api/state.mjs` around lines 104 - 109, createMockId currently uses Date.now() and Math.random(), making IDs non-deterministic; replace those entropy sources so the suffix is derived only from behaviorSeed() and the sequence to make IDs seed-stable. Modify createMockId (use nextMockSequence() once into a local sequence variable) and compute the hash from a deterministic string such as `${prefix}:${behaviorSeed()}:${sequence}` (then toString(16).slice(0,8) as before) so the returned ID is stable for a given seed and sequence.scripts/mock-api/server.mjs (1)
115-131: ⚡ Quick winMatch
rule.pathagainst the pathname, not the full URL.
ctx.urlstill contains the query string, so a rule like{ path: "/api/conversations" }will miss/api/conversations?limit=20. Parsing once and comparingpathnamehere makes exact-match fault rules behave predictably.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mock-api/server.mjs` around lines 115 - 131, ruleMatches currently compares rule.path, rule.pathRegex, and rule.contains against ctx.url which includes query strings; change it to parse ctx.url once into a pathname (e.g. const pathname = new URL(ctx.url, "http://localhost").pathname) at the top of ruleMatches and use pathname instead of ctx.url for the exact-match (rule.path), regex test (rule.pathRegex) and containment checks (rule.contains) so rules like { path: "/api/conversations" } match "/api/conversations?limit=20"; keep the existing try/catch around new RegExp(rule.pathRegex) and leave method handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mock-api/admin.mjs`:
- Around line 63-74: The /__admin/reset handler currently calls
resetSocketSessions() but does not clear in-memory socket events, causing
socketEventCount and event reads to leak across tests; add a call to the routine
that clears the socket event log (e.g., resetSocketEvents() or
clearSocketEventLog()) in the same block—either implement resetSocketEvents() to
empty the socket events array/log and then invoke it alongside
resetSocketSessions(), or directly clear the socket events storage (e.g.,
socketEvents = [] or socketEventLog.clear()) inside the POST "/__admin/reset"
branch to ensure deterministic isolation.
In `@scripts/mock-api/routes/conversations.mjs`:
- Around line 12-16: The fixture reseeding logic in ensureConversationFixtures()
(which calls getMockConversations() and getMockMessages()) runs on every request
and repopulates data when conversations are empty; change it to only seed once
at process startup or when an explicit "seed" action is requested. Concretely,
add a module-level boolean (e.g., seeded = false) or an env/config flag and
modify ensureConversationFixtures() to return immediately if seeded is true, set
seeded = true after the first successful seed, and remove/avoid calling
ensureConversationFixtures() from per-request code paths; alternatively move the
initial call to a startup/init routine so deleting all conversations remains
durable across requests.
- Around line 75-87: The current create logic builds `created` by generating
server-controlled fields (e.g., `id` via `createMockId("conv")`, `createdAt`,
`updatedAt`, `archived`, `unreadCount`, `channel`, `title`) and then spreads
`parsedBody`, which allows clients to overwrite those fields; change this to
either whitelist allowed client-updatable fields or spread `parsedBody` first
and then explicitly set/override server fields so they cannot be clobbered.
Update the `created` construction (and the duplicate create path later that also
uses `parsedBody`) to ignore or pick only safe keys from `parsedBody` (e.g.,
title/channel) before merging, and always set `id`, `createdAt`, `updatedAt`,
`archived`, and `unreadCount` after merging to ensure server control.
- Around line 118-125: The DELETE handler currently only removes the entry from
the conversations array and always returns { deleted: true }; update the logic
in the method === "DELETE" branch (use conversationItemMatch[1] as the id) to
first check whether a conversation existed (use conversations.findIndex or
similar), set deleted = index >= 0, if deleted remove the conversation and also
remove any orphaned messages from the messages array (filter out messages with
message.conversationId === id), and return json(res, 200, { success: true, data:
{ deleted } }) (or a 404 when preferred) so the response accurately reflects
whether a deletion occurred.
In `@scripts/mock-api/routes/cron.mjs`:
- Around line 62-66: The POST and PATCH handlers currently merge parsedBody into
entities allowing callers to set/overwrite id (e.g., in the created object built
with createMockId and ...(parsedBody || {})), which can break lookups; fix by
stripping id from parsedBody before merging (or always assign id from
createMockId after the merge) in the create flow, and in the PATCH/update flow
ignore any parsedBody.id and only apply other fields to the existing entity (or
validate that parsedBody.id matches the route id and reject otherwise); update
the code paths that build created and the patch/update functions to ensure id
cannot be mutated.
- Around line 11-15: The seeding in ensureCronFixtures (which calls
getMockCronJobs, getMockWebhookTriggers and fuzzyNumber) runs on every request
when arrays are empty, so full deletions are immediately repopulated; to fix it
add a durable "seeded" guard (e.g., a module-level boolean like
seededCronFixtures) and only run the seeding logic once (check
seededCronFixtures before creating fixtures and set it true after seeding), and
apply the same guarded behavior to the other identical block referenced at lines
53-55 so deletions persist across requests.
In `@scripts/mock-api/server.mjs`:
- Around line 91-97: The Socket.IO path short-circuits before global behaviors
run: move the call to maybeApplyGlobalBehavior(ctx) so it runs before the
url.startsWith("/socket.io/") check; call const maybeShortCircuit = await
maybeApplyGlobalBehavior(ctx) first and return if truthy, then call
handleSocketRequest(ctx) for socket routes (use the same
url.startsWith("/socket.io/") condition) so polling handshakes and poll cycles
are also subject to the global delay/fault layer.
In `@scripts/mock-api/socket.mjs`:
- Around line 267-278: The connect_error path currently sends the error packet
but leaves the session in socketSessions, leaking sessions; after sending the
connect error (in the branch handling packet.startsWith("40") where
authenticateSession(auth) returns !result.ok and you call sendSocketPacket and
appendSocketEvent with socketConnectErrorPacket and session.sid), ensure you
teardown the rejected session by removing it from socketSessions,
decrementing/adjusting any activeSessionCount and performing any per-session
cleanup (clear timers, remove from other registries) for that session.sid so the
rejected session is not retained; update the branch after appendSocketEvent to
call the teardown logic (same code used for normal disconnects) or a new helper
(e.g., removeSocketSession(session.sid) / cleanupSession(session)) to centralize
removal and avoid duplicate code.
---
Outside diff comments:
In `@scripts/mock-api/routes/cron.mjs`:
- Around line 83-89: The response currently sets deleted based on the HTTP
method instead of whether a matching record was actually removed; update the
logic in the handler around cronJobs.splice and the json(...) response so
deleted is true only when a record was found and removed (e.g., use index >= 0
or a removed flag set when cronJobs.splice runs) and return the actual id from
cronItem when present; apply the same fix to the duplicate block referencing
cronJobs.splice / cronItem around the later section (lines 124-130) so both
endpoints report deletion status correctly.
---
Nitpick comments:
In `@scripts/mock-api/server.mjs`:
- Around line 115-131: ruleMatches currently compares rule.path, rule.pathRegex,
and rule.contains against ctx.url which includes query strings; change it to
parse ctx.url once into a pathname (e.g. const pathname = new URL(ctx.url,
"http://localhost").pathname) at the top of ruleMatches and use pathname instead
of ctx.url for the exact-match (rule.path), regex test (rule.pathRegex) and
containment checks (rule.contains) so rules like { path: "/api/conversations" }
match "/api/conversations?limit=20"; keep the existing try/catch around new
RegExp(rule.pathRegex) and leave method handling unchanged.
In `@scripts/mock-api/state.mjs`:
- Around line 104-109: createMockId currently uses Date.now() and Math.random(),
making IDs non-deterministic; replace those entropy sources so the suffix is
derived only from behaviorSeed() and the sequence to make IDs seed-stable.
Modify createMockId (use nextMockSequence() once into a local sequence variable)
and compute the hash from a deterministic string such as
`${prefix}:${behaviorSeed()}:${sequence}` (then toString(16).slice(0,8) as
before) so the returned ID is stable for a given seed and sequence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b343fd87-3fb2-4c56-ba67-fcca30be0fd1
📒 Files selected for processing (9)
scripts/mock-api/admin.mjsscripts/mock-api/index.mjsscripts/mock-api/routes/__tests__/stateful.test.mjsscripts/mock-api/routes/conversations.mjsscripts/mock-api/routes/cron.mjsscripts/mock-api/server.mjsscripts/mock-api/socket.mjsscripts/mock-api/socket.test.mjsscripts/mock-api/state.mjs
Comment on lines
63
to
74
| if (method === "POST" && /^\/__admin\/reset\/?$/.test(url)) { | ||
| const keepBehavior = parsedBody?.keepBehavior === true; | ||
| const keepRequests = parsedBody?.keepRequests === true; | ||
| if (!keepBehavior) resetMockBehavior(); | ||
| if (!keepRequests) clearRequestLog(); | ||
| resetMockTunnels(); | ||
| resetMockConversations(); | ||
| resetMockMessages(); | ||
| resetMockCronJobs(); | ||
| resetMockWebhookTriggers(); | ||
| resetSocketSessions(); | ||
| json(res, 200, { |
Contributor
There was a problem hiding this comment.
Global reset should also clear socket event log for deterministic test isolation.
/__admin/reset resets sessions and other collections but leaves prior socket events in memory, so socketEventCount/event reads can leak across tests.
Suggested fix
if (!keepBehavior) resetMockBehavior();
if (!keepRequests) clearRequestLog();
resetMockTunnels();
resetMockConversations();
resetMockMessages();
resetMockCronJobs();
resetMockWebhookTriggers();
resetSocketSessions();
+ clearSocketEventLog();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/mock-api/admin.mjs` around lines 63 - 74, The /__admin/reset handler
currently calls resetSocketSessions() but does not clear in-memory socket
events, causing socketEventCount and event reads to leak across tests; add a
call to the routine that clears the socket event log (e.g., resetSocketEvents()
or clearSocketEventLog()) in the same block—either implement resetSocketEvents()
to empty the socket events array/log and then invoke it alongside
resetSocketSessions(), or directly clear the socket events storage (e.g.,
socketEvents = [] or socketEventLog.clear()) inside the POST "/__admin/reset"
branch to ensure deterministic isolation.
Comment on lines
118
to
125
| if (method === "DELETE") { | ||
| const index = conversations.findIndex( | ||
| (entry) => entry.id === conversationItemMatch[1], | ||
| ); | ||
| if (index >= 0) { | ||
| conversations.splice(index, 1); | ||
| } | ||
| json(res, 200, { success: true, data: { deleted: true } }); |
Contributor
There was a problem hiding this comment.
Conversation delete leaves orphan messages and misreports deletion.
Delete only removes the conversation, not its messages, and response always returns deleted: true even when the id is absent.
Suggested fix
if (method === "DELETE") {
const index = conversations.findIndex(
(entry) => entry.id === conversationItemMatch[1],
);
+ let deleted = false;
if (index >= 0) {
+ const removedId = conversations[index].id;
conversations.splice(index, 1);
+ deleted = true;
+ for (let i = messages.length - 1; i >= 0; i -= 1) {
+ if (messages[i].conversationId === removedId) messages.splice(i, 1);
+ }
}
- json(res, 200, { success: true, data: { deleted: true } });
+ json(res, 200, { success: true, data: { deleted } });
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (method === "DELETE") { | |
| const index = conversations.findIndex( | |
| (entry) => entry.id === conversationItemMatch[1], | |
| ); | |
| if (index >= 0) { | |
| conversations.splice(index, 1); | |
| } | |
| json(res, 200, { success: true, data: { deleted: true } }); | |
| if (method === "DELETE") { | |
| const index = conversations.findIndex( | |
| (entry) => entry.id === conversationItemMatch[1], | |
| ); | |
| let deleted = false; | |
| if (index >= 0) { | |
| const removedId = conversations[index].id; | |
| conversations.splice(index, 1); | |
| deleted = true; | |
| for (let i = messages.length - 1; i >= 0; i -= 1) { | |
| if (messages[i].conversationId === removedId) messages.splice(i, 1); | |
| } | |
| } | |
| json(res, 200, { success: true, data: { deleted } }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/mock-api/routes/conversations.mjs` around lines 118 - 125, The DELETE
handler currently only removes the entry from the conversations array and always
returns { deleted: true }; update the logic in the method === "DELETE" branch
(use conversationItemMatch[1] as the id) to first check whether a conversation
existed (use conversations.findIndex or similar), set deleted = index >= 0, if
deleted remove the conversation and also remove any orphaned messages from the
messages array (filter out messages with message.conversationId === id), and
return json(res, 200, { success: true, data: { deleted } }) (or a 404 when
preferred) so the response accurately reflects whether a deletion occurred.
Comment on lines
+267
to
+278
| if (packet.startsWith("40")) { | ||
| const auth = normalizeAuthPayload(packet); | ||
| const result = authenticateSession(auth); | ||
| if (!result.ok) { | ||
| sendSocketPacket(session, socketConnectErrorPacket(result.message)); | ||
| appendSocketEvent({ | ||
| direction: "outbound", | ||
| kind: "connect_error", | ||
| sid: session.sid, | ||
| message: result.message, | ||
| }); | ||
| return; |
Contributor
There was a problem hiding this comment.
Clean up rejected sessions after connect_error.
This path sends/queues the error packet but keeps the session in socketSessions forever. Repeated auth-failure tests will accumulate stale sessions, inflate activeSessionCount, and make later admin/event operations see dead sockets. Please add a post-delivery teardown path for rejected sessions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/mock-api/socket.mjs` around lines 267 - 278, The connect_error path
currently sends the error packet but leaves the session in socketSessions,
leaking sessions; after sending the connect error (in the branch handling
packet.startsWith("40") where authenticateSession(auth) returns !result.ok and
you call sendSocketPacket and appendSocketEvent with socketConnectErrorPacket
and session.sid), ensure you teardown the rejected session by removing it from
socketSessions, decrementing/adjusting any activeSessionCount and performing any
per-session cleanup (clear timers, remove from other registries) for that
session.sid so the rejected session is not retained; update the branch after
appendSocketEvent to call the teardown logic (same code used for normal
disconnects) or a new helper (e.g., removeSocketSession(session.sid) /
cleanupSession(session)) to centralize removal and avoid duplicate code.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/mock-api/socket/core.mjs (1)
194-205:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRejected auth sessions are retained after
connect_error.Line 197 sends/queues
connect_error, but this branch returns without any teardown marker/path. Rejected sessions can accumulate and pollute session-based admin/state operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mock-api/socket/core.mjs` around lines 194 - 205, The auth-rejection path for packet.startsWith("40") currently sends a connect_error but returns without cleaning up the session; update the branch handling authenticateSession(auth) failure to perform proper teardown: call the existing session teardown/cleanup function (or explicitly close the socket and remove the session from the session store), mark the session as rejected/closed (so session.sid is not retained for admin/state ops), and then append the connect_error event via appendSocketEvent and return; locate this logic around normalizeAuthPayload, authenticateSession, sendSocketPacket, appendSocketEvent and ensure the same cleanup is used elsewhere to avoid duplicating teardown logic.
🧹 Nitpick comments (1)
scripts/mock-api/socket/websocket.mjs (1)
94-97: 💤 Low valueConsider sending a close frame before ending the socket.
RFC 6455 Section 5.5.1 specifies that when receiving a close frame, the server should send a close frame in response before closing. Current code calls
socket.end()without responding. For a mock server this likely won't affect test behavior, but if any tests validate proper close handshake, this could be relevant.♻️ Optional: RFC-compliant close response
if (opcode === 0x08) { + sendWsFrame(socket, 0x08, payload); socket.end(); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mock-api/socket/websocket.mjs` around lines 94 - 97, When handling an incoming close opcode (the opcode === 0x08 branch) send a WebSocket close frame back to the client before closing the connection: construct and write a close frame (opcode 0x08) to the socket, echoing the received close payload if present, then call socket.end(); update the code around the opcode === 0x08 check to write the close frame (via the existing frame-send utility or socket.write) prior to socket.end() so the server performs the RFC‑compliant close handshake.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mock-api/socket/core.mjs`:
- Around line 334-337: The close handler currently always calls
dropSocketSession(session.sid); change it to only drop the session if the
WebSocket upgrade was fully finalized: add/consult an upgrade flag (e.g.
session.upgradeComplete or session.upgraded) that is set to true when the "5"
upgrade packet is received/processed in your packet handling logic, then in
socket.on("close", ...) check that flag before calling
dropSocketSession(session.sid) and still call
logSocketCheckpoint("websocket_closed", { sid: session.sid }); regardless;
update the upgrade-packet handler to set the same flag so the check is
effective.
---
Duplicate comments:
In `@scripts/mock-api/socket/core.mjs`:
- Around line 194-205: The auth-rejection path for packet.startsWith("40")
currently sends a connect_error but returns without cleaning up the session;
update the branch handling authenticateSession(auth) failure to perform proper
teardown: call the existing session teardown/cleanup function (or explicitly
close the socket and remove the session from the session store), mark the
session as rejected/closed (so session.sid is not retained for admin/state ops),
and then append the connect_error event via appendSocketEvent and return; locate
this logic around normalizeAuthPayload, authenticateSession, sendSocketPacket,
appendSocketEvent and ensure the same cleanup is used elsewhere to avoid
duplicating teardown logic.
---
Nitpick comments:
In `@scripts/mock-api/socket/websocket.mjs`:
- Around line 94-97: When handling an incoming close opcode (the opcode === 0x08
branch) send a WebSocket close frame back to the client before closing the
connection: construct and write a close frame (opcode 0x08) to the socket,
echoing the received close payload if present, then call socket.end(); update
the code around the opcode === 0x08 check to write the close frame (via the
existing frame-send utility or socket.write) prior to socket.end() so the server
performs the RFC‑compliant close handshake.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bd61fae-97c7-4c9f-a13b-fa931a953b9f
📒 Files selected for processing (8)
scripts/mock-api/socket.auth.test.mjsscripts/mock-api/socket.mjsscripts/mock-api/socket.transport.test.mjsscripts/mock-api/socket/core.mjsscripts/mock-api/socket/index.mjsscripts/mock-api/socket/protocol.mjsscripts/mock-api/socket/websocket.mjsscripts/mock-api/test-helpers/socket-client.mjs
✅ Files skipped from review due to trivial changes (1)
- scripts/mock-api/socket/index.mjs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/mock-api/routes/cron.mjs (1)
83-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn not-found semantics for missing PATCH/DELETE targets.
At Line 97 and Line 139, DELETE reports success even when the
iddoes not exist. PATCH also silently succeeds on misses. This hides incorrect client behavior in tests.💡 Suggested fix
if (cronItem && (method === "PATCH" || method === "DELETE")) { const index = cronJobs.findIndex((entry) => entry.id === cronItem[1]); + const found = index >= 0; + if (!found) { + json(res, 404, { success: false, error: "Cron job not found" }); + return true; + } - if (index >= 0 && method === "PATCH") { + if (method === "PATCH") { const { id: _ignoredId, ...patch } = parsedBody || {}; cronJobs[index] = { ...cronJobs[index], ...patch, }; } - if (index >= 0 && method === "DELETE") { + if (method === "DELETE") { cronJobs.splice(index, 1); } @@ if (trgItem && (method === "PATCH" || method === "DELETE")) { const index = webhookTriggers.findIndex((entry) => entry.id === trgItem[1]); - if (index >= 0 && method === "PATCH") { + const found = index >= 0; + if (!found) { + json(res, 404, { success: false, error: "Webhook trigger not found" }); + return true; + } + if (method === "PATCH") { const { id: _ignoredId, ...patch } = parsedBody || {}; webhookTriggers[index] = { ...webhookTriggers[index], ...patch, }; } - if (index >= 0 && method === "DELETE") { + if (method === "DELETE") { webhookTriggers.splice(index, 1); }Also applies to: 125-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mock-api/routes/cron.mjs` around lines 83 - 99, The current handler treats PATCH/DELETE as successful even when the target id isn't found; update the logic around cronItem, cronJobs and method to check the found index and return a not-found response when index < 0: for PATCH, do not apply the patch and instead call json(res, 404, { success: false, error: "Not found", data: { id: cronItem[1] } }); for DELETE, do not splice and return the same 404 semantics; keep the existing 200 success response only when index >= 0 (and include deleted: true for successful DELETE).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/mock-api/routes/llm/dynamic.mjs`:
- Around line 98-103: The generator emits an `await` without ensuring the
function is `async` when language === "js"; update the logic to compute a single
isAsync flag (e.g., const isAsync = /async/.test(lower) || language === "ts")
and use that flag for both asyncKeyword and awaitLine so asyncKeyword and
awaitLine stay in sync (affecting the variables asyncKeyword and awaitLine used
to build runTask); this ensures JS snippets only include `await` when the
function is declared async.
In `@scripts/mock-api/server.mjs`:
- Around line 152-153: Validate and sanitize the injected fault status before
using it: read rule.status into a local variable (e.g., statusRaw), parse it to
an integer and ensure it's finite and within HTTP range 100–599; if it's invalid
(NaN, non-integer, or out of range) fall back to 500. Replace the direct
Number(rule.status || 500) logic around the status and subsequent response write
in the request handler so the code uses the validated/clamped integer
(referencing the variables rule and status/body) to avoid thrown errors during
response writes.
In `@scripts/mock-api/state.mjs`:
- Around line 483-485: Generate and reuse the daily limit value instead of
calling fuzzyNumber twice: create a variable (e.g., dailyTokenLimit) by calling
fuzzyNumber("team:dailyTokenLimit", 500, 5000) and then set remainingTokens so
it cannot exceed that value (e.g., call fuzzyNumber with max = dailyTokenLimit
or clamp the result with Math.min), keeping activeSessionCount as
listSocketSessions().length; update the assignments for dailyTokenLimit and
remainingTokens in the same block so remainingTokens <= dailyTokenLimit.
---
Outside diff comments:
In `@scripts/mock-api/routes/cron.mjs`:
- Around line 83-99: The current handler treats PATCH/DELETE as successful even
when the target id isn't found; update the logic around cronItem, cronJobs and
method to check the found index and return a not-found response when index < 0:
for PATCH, do not apply the patch and instead call json(res, 404, { success:
false, error: "Not found", data: { id: cronItem[1] } }); for DELETE, do not
splice and return the same 404 semantics; keep the existing 200 success response
only when index >= 0 (and include deleted: true for successful DELETE).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68a64acb-9d77-4085-b62f-7240e457ce3c
📒 Files selected for processing (15)
scripts/mock-api/admin.mjsscripts/mock-api/index.mjsscripts/mock-api/routes/__tests__/llm.test.mjsscripts/mock-api/routes/__tests__/stateful.test.mjsscripts/mock-api/routes/conversations.mjsscripts/mock-api/routes/cron.mjsscripts/mock-api/routes/integrations.mjsscripts/mock-api/routes/llm.mjsscripts/mock-api/routes/llm/dynamic.mjsscripts/mock-api/routes/llm/shared.mjsscripts/mock-api/server.mjsscripts/mock-api/socket.auth.test.mjsscripts/mock-api/socket.transport.test.mjsscripts/mock-api/socket/core.mjsscripts/mock-api/state.mjs
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/mock-api/socket.auth.test.mjs
- scripts/mock-api/index.mjs
- scripts/mock-api/admin.mjs
- scripts/mock-api/routes/conversations.mjs
- scripts/mock-api/socket/core.mjs
Comment on lines
+98
to
+103
| const asyncKeyword = language === "ts" ? "async " : ""; | ||
| const typeSuffix = language === "ts" ? ": Promise<string>" : ""; | ||
| const awaitLine = /async/.test(lower) ? " await Promise.resolve();\n" : ""; | ||
| const testBlock = /test/.test(lower) | ||
| ? `\nexport function runTaskTest() {\n return runTask("build");\n}\n` | ||
| : "\n"; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/mock-api/routes/llm/dynamic.mjs | head -120Repository: tinyhumansai/openhuman
Length of output: 4299
Fix invalid JavaScript snippet generation for async prompts.
Line 100 can emit await while line 106 still emits a non-async function when language === "js", producing invalid syntax. JavaScript await requires an async context, but asyncKeyword on line 98 is tied only to language === "ts". When a prompt contains "async" but language is JavaScript, the generated code becomes function runTask(...) { await Promise.resolve(); }, which is invalid. See how Python (line 41) and Rust (line 63) handle this correctly by gating both the async keyword and await statement on the same check.
💡 Suggested fix
- const asyncKeyword = language === "ts" ? "async " : "";
- const typeSuffix = language === "ts" ? ": Promise<string>" : "";
- const awaitLine = /async/.test(lower) ? " await Promise.resolve();\n" : "";
+ const isAsync = /async/.test(lower);
+ const asyncKeyword = isAsync ? "async " : "";
+ const typeSuffix =
+ language === "ts" ? (isAsync ? ": Promise<string>" : ": string") : "";
+ const awaitLine = isAsync ? " await Promise.resolve();\n" : "";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const asyncKeyword = language === "ts" ? "async " : ""; | |
| const typeSuffix = language === "ts" ? ": Promise<string>" : ""; | |
| const awaitLine = /async/.test(lower) ? " await Promise.resolve();\n" : ""; | |
| const testBlock = /test/.test(lower) | |
| ? `\nexport function runTaskTest() {\n return runTask("build");\n}\n` | |
| : "\n"; | |
| const isAsync = /async/.test(lower); | |
| const asyncKeyword = isAsync ? "async " : ""; | |
| const typeSuffix = | |
| language === "ts" ? (isAsync ? ": Promise<string>" : ": string") : ""; | |
| const awaitLine = isAsync ? " await Promise.resolve();\n" : ""; | |
| const testBlock = /test/.test(lower) | |
| ? `\nexport function runTaskTest() {\n return runTask("build");\n}\n` | |
| : "\n"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/mock-api/routes/llm/dynamic.mjs` around lines 98 - 103, The generator
emits an `await` without ensuring the function is `async` when language ===
"js"; update the logic to compute a single isAsync flag (e.g., const isAsync =
/async/.test(lower) || language === "ts") and use that flag for both
asyncKeyword and awaitLine so asyncKeyword and awaitLine stay in sync (affecting
the variables asyncKeyword and awaitLine used to build runTask); this ensures JS
snippets only include `await` when the function is declared async.
Comment on lines
+152
to
+153
| const status = Number(rule.status || 500); | ||
| const body = |
Contributor
There was a problem hiding this comment.
Validate injected fault status before writing the response.
Line 152 currently accepts NaN or out-of-range values from rule.status, which can throw during response write and break intended fault-injection behavior. Add a guarded integer fallback.
Suggested fix
- const status = Number(rule.status || 500);
+ const parsedStatus = Number(rule.status);
+ const status =
+ Number.isInteger(parsedStatus) && parsedStatus >= 100 && parsedStatus <= 599
+ ? parsedStatus
+ : 500;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const status = Number(rule.status || 500); | |
| const body = | |
| const parsedStatus = Number(rule.status); | |
| const status = | |
| Number.isInteger(parsedStatus) && parsedStatus >= 100 && parsedStatus <= 599 | |
| ? parsedStatus | |
| : 500; | |
| const body = |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/mock-api/server.mjs` around lines 152 - 153, Validate and sanitize
the injected fault status before using it: read rule.status into a local
variable (e.g., statusRaw), parse it to an integer and ensure it's finite and
within HTTP range 100–599; if it's invalid (NaN, non-integer, or out of range)
fall back to 500. Replace the direct Number(rule.status || 500) logic around the
status and subsequent response write in the request handler so the code uses the
validated/clamped integer (referencing the variables rule and status/body) to
avoid thrown errors during response writes.
Comment on lines
+483
to
+485
| dailyTokenLimit: fuzzyNumber("team:dailyTokenLimit", 500, 5000), | ||
| remainingTokens: fuzzyNumber("team:remainingTokens", 100, 5000), | ||
| activeSessionCount: listSocketSessions().length, |
Contributor
There was a problem hiding this comment.
Bound remainingTokens to the generated token limit.
Line 484 can exceed Line 483, producing impossible usage state (remainingTokens > dailyTokenLimit).
💡 Suggested fix
export function getMockTeam() {
@@
const teamName =
@@
"Personal",
);
+ const dailyTokenLimit = fuzzyNumber("team:dailyTokenLimit", 500, 5000);
+ const remainingTokens = fuzzyNumber("team:remainingTokens", 0, dailyTokenLimit);
return {
@@
usage: {
- dailyTokenLimit: fuzzyNumber("team:dailyTokenLimit", 500, 5000),
- remainingTokens: fuzzyNumber("team:remainingTokens", 100, 5000),
+ dailyTokenLimit,
+ remainingTokens,
activeSessionCount: listSocketSessions().length,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dailyTokenLimit: fuzzyNumber("team:dailyTokenLimit", 500, 5000), | |
| remainingTokens: fuzzyNumber("team:remainingTokens", 100, 5000), | |
| activeSessionCount: listSocketSessions().length, | |
| const dailyTokenLimit = fuzzyNumber("team:dailyTokenLimit", 500, 5000); | |
| const remainingTokens = fuzzyNumber("team:remainingTokens", 0, dailyTokenLimit); | |
| // ... (other code in getMockTeam function) | |
| usage: { | |
| dailyTokenLimit, | |
| remainingTokens, | |
| activeSessionCount: listSocketSessions().length, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/mock-api/state.mjs` around lines 483 - 485, Generate and reuse the
daily limit value instead of calling fuzzyNumber twice: create a variable (e.g.,
dailyTokenLimit) by calling fuzzyNumber("team:dailyTokenLimit", 500, 5000) and
then set remainingTokens so it cannot exceed that value (e.g., call fuzzyNumber
with max = dailyTokenLimit or clamp the result with Math.min), keeping
activeSessionCount as listSocketSessions().length; update the assignments for
dailyTokenLimit and remainingTokens in the same block so remainingTokens <=
dailyTokenLimit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Problem
socket.io-clientbehavior used by the appSolution
ready, polling, websocket upgrade, server-pushed events, and connect errorsSubmission Checklist
## Related— N/A: no matrix-backed feature row changes for this harness-only updatedocs/RELEASE-MANUAL-SMOKE.md) — N/A: test harness onlyCloses #NNNin the## Relatedsection — N/A: no linked issue was provided for this requestImpact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/mock-server-socket-fuzzf05903699dacff9351b0341dadf9a8ad7311c2efValidation Run
pnpm --filter openhuman-app format:checkpnpm typechecknode --test scripts/mock-api/socket.test.mjs scripts/mock-api/routes/__tests__/stateful.test.mjs scripts/mock-api/routes/__tests__/llm.test.mjscargo fmt --manifest-path ../Cargo.toml --all --checkandcargo fmt --manifest-path app/src-tauri/Cargo.toml --all --checkran viapnpm --filter openhuman-app format:check;cargo check --manifest-path src-tauri/Cargo.tomlran via pre-pushcargo check --manifest-path src-tauri/Cargo.tomlran via pre-pushValidation Blocked
command: node scripts/codex-pr-preflight.mjs --strict-path --lightweighterror: [FAIL] expected repo path :: expected /workspace/openhuman, got /Users/enamakel/work/tinyhumansai/openhuman-1; [FAIL] branch naming convention :: codex/mock-server-socket-fuzzimpact: environment-specific Codex preflight could not fully pass, but branch push and focused validations completed successfullyBehavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Tests