Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/dashboard-server/src/lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ export function sendHtmlFileOrFallback(
statusIfMissing = 404,
): void {
if (fs.existsSync(filePath)) {
res.sendFile(path.resolve(filePath));
res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8'));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using synchronous file system operations (fs.readFileSync) inside an Express request handler or response helper blocks the Node.js event loop. This can severely degrade server performance and throughput under concurrent load.

Unless there is a specific reason to avoid res.sendFile, it is highly recommended to keep using it as it is asynchronous, non-blocking, and handles streaming and caching headers efficiently. If you must read the file manually, please use asynchronous file system methods (e.g., fs.promises.readFile).

Suggested change
res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8'));
res.sendFile(path.resolve(filePath));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 File serving changed from async to synchronous, losing Express built-in features

The change from res.sendFile(path.resolve(filePath)) to res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8')) has several behavioral implications:

  1. Blocking I/O: fs.readFileSync blocks the event loop during file reads. For small HTML files in a dashboard this is unlikely to matter, but it's a regression from the async sendFile.
  2. Lost Express features: res.sendFile() automatically handles ETag generation, Last-Modified headers, conditional GET (304 responses), and graceful error handling (calls next(err) on failure). The new code loses all of these.
  3. Error handling: If the file is deleted between fs.existsSync (line 219) and fs.readFileSync (line 220), the old code would trigger Express error handling; the new code throws an unhandled exception that could crash the process.

For a dashboard serving a handful of small HTML files, these are unlikely to cause real problems, but the tradeoffs should be intentional.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Replacing Express sendFile() with fs.readFileSync() on the request path blocks the event loop and buffers the entire file into memory before sending, which is a performance regression on frequently hit routes like /app and /metrics compared to the prior streaming behavior.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/dashboard-server/src/lib/utils.ts, line 220:

<comment>Replacing Express `sendFile()` with `fs.readFileSync()` on the request path blocks the event loop and buffers the entire file into memory before sending, which is a performance regression on frequently hit routes like `/app` and `/metrics` compared to the prior streaming behavior.</comment>

<file context>
@@ -217,7 +217,7 @@ export function sendHtmlFileOrFallback(
 ): void {
   if (fs.existsSync(filePath)) {
-    res.sendFile(path.resolve(filePath));
+    res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8'));
     return;
   }
</file context>
Suggested change
res.type('html').send(fs.readFileSync(path.resolve(filePath), 'utf-8'));
res.sendFile(path.resolve(filePath));

return;
Comment on lines 219 to 221

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the relevant files and inspect the function/call sites with line numbers.
git ls-files packages/dashboard-server/src/lib/utils.ts packages/dashboard-server/src/proxy-server.ts
echo "---- utils.ts outline ----"
ast-grep outline packages/dashboard-server/src/lib/utils.ts --view expanded || true
echo "---- proxy-server.ts outline ----"
ast-grep outline packages/dashboard-server/src/proxy-server.ts --view expanded || true
echo "---- utils.ts relevant lines ----"
sed -n '180,260p' packages/dashboard-server/src/lib/utils.ts | cat -n
echo "---- proxy-server.ts relevant routes ----"
sed -n '430,520p' packages/dashboard-server/src/proxy-server.ts | cat -n

Repository: AgentWorkforce/relay-dashboard

Length of output: 9779


🌐 Web query:

Express res.sendFile documentation ETag Last-Modified range handling callback

💡 Result:

The res.sendFile method in Express is used to transfer a file at a given path, automatically setting the Content-Type header based on the file extension [1][2]. ETag and Last-Modified Handling Express automatically handles ETag generation and Last-Modified headers when using res.sendFile [3][4]. These mechanisms allow browsers and clients to perform conditional requests, improving performance by validating whether a cached version of the file is still current. Range Requests The method includes built-in support for HTTP Range requests [4]. This allows clients to request specific portions of a file, which is essential for tasks like resuming interrupted downloads or streaming media. Support for range requests is enabled by default [5]. In the options object, you can configure the acceptRanges property (defaults to true) to control this behavior [1][2]. Callback Function The res.sendFile method accepts an optional callback function, fn(err), which is invoked when the transfer is complete or if an error occurs [1][6]. Important Behaviors: 1. Error Handling: If a callback is provided and an error occurs, you are responsible for handling the response process [1][2]. This means you must explicitly end the request-response cycle (e.g., via res.end or res.status.send) or pass control to the next route using next(err) [1][2]. 2. Default Behavior: If you do not provide a callback, Express will automatically pass errors to your error-handling middleware via next(err) [2][4]. Usage Example: app.get('/file/:name', (req, res, next) => { const options = { root: path.join(dirname, 'public'), dotfiles: 'deny' }; res.sendFile(req.params.name, options, (err) => { if (err) { // You must handle the error explicitly when a callback is provided next(err); } else { console.log('Transfer complete'); } }); }); [1], [2] Official Express API documentation for res.sendFile [3] Express repository documentation on ETag support [5] Express release notes regarding range handling [4] Technical overview of file operation error handling in Express

Citations:


Avoid synchronous reads for these HTML responses. res.sendFile() streams the file and preserves ETag/Last-Modified/Range handling, while fs.readFileSync() blocks the event loop on every request and re-reads from disk. Switch back to sendFile() with an error callback for the fallback, or use async I/O plus caching.

🧰 Tools
🪛 ast-grep (0.44.1)

[warning] 219-219: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.resolve(filePath), 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(detect-non-literal-fs-filename-typescript)

🤖 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 `@packages/dashboard-server/src/lib/utils.ts` around lines 219 - 221, The HTML
fallback path is using a synchronous disk read via fs.readFileSync, which blocks
the event loop and bypasses streaming/caching behavior. Update the response
logic in the utils.ts HTML-serving handler to use res.sendFile() again for the
filePath fallback, with an error callback for missing/failed files, or replace
the sync read with async I/O plus caching if sendFile is not suitable. Keep the
existing existence check flow but remove the synchronous read/send pattern.

}

Expand Down
4 changes: 3 additions & 1 deletion packages/dashboard-server/src/mocks/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export function registerMockRoutes(app: Express, verbose: boolean): void {
});

app.post('/api/spawn', (req: Request, res: Response) => {
const { name, cli = 'claude-code', task } = req.body || {};
const { name, cli = 'claude-code', task, continueFrom } = req.body || {};
log(`POST /api/spawn - ${name}`);

if (!name) {
Expand All @@ -96,6 +96,8 @@ export function registerMockRoutes(app: Express, verbose: boolean): void {
name,
cli,
task,
continueFrom,
resumed: typeof continueFrom === 'string' && continueFrom.length > 0,
message: `Agent ${name} spawned successfully (mock)`,
});
});
Expand Down
75 changes: 75 additions & 0 deletions packages/dashboard-server/src/proxy-server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,81 @@ describe('Dashboard Server', () => {
});
});

describe('Broker Proxy Mode', () => {
let brokerServer: HttpServer;
let dashboardServer: DashboardServer;
let forwardedSpawnBody: unknown;

beforeAll(async () => {
brokerServer = createHttpServer((req, res) => {
if (req.method !== 'POST' || req.url !== '/api/spawn') {
res.writeHead(404);
res.end();
return;
}

let body = '';
req.on('data', (chunk) => {
body += chunk;
});
req.on('end', () => {
forwardedSpawnBody = JSON.parse(body);
res.writeHead(200, { 'content-type': 'application/json' });
res.end(JSON.stringify({ success: true }));
});
});
await new Promise<void>((resolve) => {
brokerServer.listen(0, () => resolve());
});

const brokerAddress = brokerServer.address();
if (!brokerAddress || typeof brokerAddress === 'string') {
throw new Error('Broker address not available');
}

dashboardServer = createServer({
port: 0,
mock: false,
relayUrl: `http://localhost:${brokerAddress.port}`,
verbose: false,
});
await new Promise<void>((resolve) => {
dashboardServer.server.listen(0, () => resolve());
});
});

afterAll(async () => {
await dashboardServer.close();
await new Promise<void>((resolve, reject) => {
brokerServer.close((err) => (err ? reject(err) : resolve()));
});
});

it('should forward continueFrom when resuming a previous session', async () => {
const address = dashboardServer.server.address();
if (!address || typeof address === 'string') {
throw new Error('Server address not available');
}

const response = await fetch(`http://localhost:${address.port}/api/spawn`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
name: 'worker',
cli: 'claude-code',
task: 'resume context',
continueFrom: 'worker',
}),
});

expect(response.ok).toBe(true);
expect(forwardedSpawnBody).toMatchObject({
name: 'worker',
continueFrom: 'worker',
});
});
});

describe('Proxy Mode (Configuration)', () => {
let server: DashboardServer;

Expand Down
2 changes: 2 additions & 0 deletions packages/dashboard-server/src/routes/broker-proxy.ts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Architect spawn route does not forward continueFrom

The /api/spawn route at packages/dashboard-server/src/routes/broker-proxy.ts:112-118 now explicitly extracts and forwards continueFrom. However, the /api/spawn/architect route at packages/dashboard-server/src/routes/broker-proxy.ts:127-144 does NOT do the same — it uses ...rawBody which would pass through continueFrom if present, but without the type validation applied to the regular spawn route. This inconsistency may be intentional (architects may not support resume) or an oversight. If the architect route should also support session resumption, it should get the same explicit handling.

(Refers to lines 127-144)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,13 @@ export function registerBrokerProxyRoutes(app: Express, ctx: RouteContext): void
typeof rawBody.model === 'string' ? rawBody.model : undefined,
);
const model = parsed.model;
const continueFrom = typeof rawBody.continueFrom === 'string' ? rawBody.continueFrom : undefined;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The explicit continueFrom type validation added here is not applied to the /api/spawn/architect route, which uses a raw body spread. If architect sessions should also support resume, the same validation should be applied there; otherwise continueFrom could pass through unvalidated. Consider whether this inconsistency is intentional.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/dashboard-server/src/routes/broker-proxy.ts, line 112:

<comment>The explicit `continueFrom` type validation added here is not applied to the `/api/spawn/architect` route, which uses a raw body spread. If architect sessions should also support resume, the same validation should be applied there; otherwise `continueFrom` could pass through unvalidated. Consider whether this inconsistency is intentional.</comment>

<file context>
@@ -109,11 +109,13 @@ export function registerBrokerProxyRoutes(app: Express, ctx: RouteContext): void
         typeof rawBody.model === 'string' ? rawBody.model : undefined,
       );
       const model = parsed.model;
+      const continueFrom = typeof rawBody.continueFrom === 'string' ? rawBody.continueFrom : undefined;
       return {
         ...rawBody,
</file context>

return {
...rawBody,
cli: parsed.cli,
args: parsed.args,
model,
continueFrom,
includeWorkflowConventions,
task: withWorkflowConventions(task, includeWorkflowConventions),
};
Expand Down
10 changes: 5 additions & 5 deletions test/bugs/bug7-resume-session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,22 +63,22 @@ describe('BUG 7 — Resume session loses context', () => {

if (res.status === 200) {
const data = await res.json();
// BUG: The response doesn't indicate whether session was resumed
// because the server never processes continueFrom
expect(data.success).toBeDefined();
expect(data.continueFrom).toBe('test-resume-agent');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The key regression assertions (continueFrom, resumed) are inside if (res.status === 200), so a non-200 response causes the test to pass silently without verifying any resume behavior. This defeats the purpose of the regression test. Assert expect(res.status).toBe(200) unconditionally before reading the body, or restructure so the assertions are not conditionally skipped.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/bugs/bug7-resume-session.test.ts, line 67:

<comment>The key regression assertions (`continueFrom`, `resumed`) are inside `if (res.status === 200)`, so a non-200 response causes the test to pass silently without verifying any resume behavior. This defeats the purpose of the regression test. Assert `expect(res.status).toBe(200)` unconditionally before reading the body, or restructure so the assertions are not conditionally skipped.</comment>

<file context>
@@ -63,22 +63,22 @@ describe('BUG 7 — Resume session loses context', () => {
-      // BUG: The response doesn't indicate whether session was resumed
-      // because the server never processes continueFrom
       expect(data.success).toBeDefined();
+      expect(data.continueFrom).toBe('test-resume-agent');
+      expect(data.resumed).toBe(true);
     }
</file context>

expect(data.resumed).toBe(true);
}
Comment on lines 64 to 69

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Bug-7 test assertions are conditional on status 200, silently passing on failure

The test at test/bugs/bug7-resume-session.test.ts:64-69 wraps its key assertions (expect(data.continueFrom).toBe(...) and expect(data.resumed).toBe(true)) inside if (res.status === 200). If the server returns any non-200 status (e.g., 501 in standalone mode, or 502 if the broker is down), the test silently passes without verifying anything. This means the test provides no guarantee that the fix actually works — it only checks the fix if the server happens to respond with 200. The test should either assert the status is 200 first, or restructure to not conditionally skip assertions.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines 66 to 69

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the target test and nearby related files.
git ls-files test/bugs/bug7-resume-session.test.ts
wc -l test/bugs/bug7-resume-session.test.ts
cat -n test/bugs/bug7-resume-session.test.ts | sed -n '1,220p'

# Find the endpoint handler and any resume-session related code paths.
rg -n "continueFrom|resumed|resume-session|res.status === 200|status === 200" test src .

Repository: AgentWorkforce/relay-dashboard

Length of output: 11363


Fail the test on non-200 responses. The continueFrom / resumed assertions are still inside if (res.status === 200), so a bad response can skip the regression check entirely. Move the status assertion outside the guard and read the body unconditionally.

🤖 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 `@test/bugs/bug7-resume-session.test.ts` around lines 66 - 69, The
resume-session test is still allowing non-200 responses to bypass the regression
checks. Update the test in bug7-resume-session.test.ts so the status assertion
is performed outside the res.status === 200 guard, and make the body
parsing/assertions for data.success, data.continueFrom, and data.resumed run
unconditionally in the relevant test block. Use the existing test case around
the resume session flow to locate and move the checks without changing the
intended expectations.

});

it('spawn.ts should destructure continueFrom from request body', () => {
it('broker-proxy.ts should explicitly forward continueFrom from request body', () => {
// This is a static code analysis test
const fs = require('fs');
const path = require('path');
const spawnRouteCode = fs.readFileSync(
path.resolve(__dirname, '../../packages/dashboard-server/src/routes/spawn.ts'),
path.resolve(__dirname, '../../packages/dashboard-server/src/routes/broker-proxy.ts'),
'utf-8'
);

// FIX: continueFrom is now read from req.body and forwarded to spawn
// FIX: continueFrom is now read from req.body and forwarded to the broker spawn endpoint
// Verify the fix is in place by checking the source contains continueFrom
expect(spawnRouteCode).toContain('continueFrom');
});
Expand Down